diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/H2Database.kt b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/H2Database.kt index 45e3b718530438473780abb3ccb9ce25e0aa6c2e..d6d2f5e9288d785506eea1998acc1bde578d75d1 100644 --- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/H2Database.kt +++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/H2Database.kt @@ -32,6 +32,7 @@ import java.sql.ResultSet import java.sql.SQLException import java.sql.Statement import java.util.Properties +import kotlin.concurrent.withLock open class H2Database( private val config: DatabaseConfig, @@ -85,8 +86,7 @@ open class H2Database( } override fun close() { - connectionsLock.lock() - try { + connectionsLock.withLock { // This extra check is mainly added for tests where we might have closed the database // already by resetting the database after each test and then the lifecycle manager // tries to close again. However closing an already closed database doesn't make @@ -106,8 +106,6 @@ open class H2Database( tryToClose(c, LOG) throw DbException(e) } - } finally { - connectionsLock.unlock() } } diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/JdbcDatabase.kt b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/JdbcDatabase.kt index bb01002a46ef1623a52cff876a4040ff233e6bd8..88399fecc562119a9336bd4a84ae72eedd618085 100644 --- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/JdbcDatabase.kt +++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/JdbcDatabase.kt @@ -43,6 +43,7 @@ import java.util.concurrent.locks.Lock import java.util.concurrent.locks.ReentrantLock import java.util.concurrent.locks.ReentrantReadWriteLock import javax.annotation.concurrent.GuardedBy +import kotlin.concurrent.withLock abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val clock: Clock) : Database { @@ -70,6 +71,20 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc """.trimIndent() } + /** + * This lock is used for making sure that either one writable or alternatively an arbitrary + * number of read-only transactions is in progress at the same time. It acts as a barrier + * that blocks threads that try to start a transaction that cannot currently be started + * because of that invariant. + */ + private val transactionLock = ReentrantReadWriteLock(true) + + /** + * In the case of readers, multiple threads can be accessing the database simultaneously. + * To synchronise access to shared fields, we use this lock. + * + * Do not start any transactions while holding [connectionsLock] as this is deadlock-prone. + */ protected val connectionsLock: Lock = ReentrantLock() private val connectionsChanged = connectionsLock.newCondition() @@ -80,15 +95,14 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc private var openConnections = 0 @GuardedBy("connectionsLock") - protected var closed = false + protected var closed = true @Volatile private var wasDirtyOnInitialisation = false - private val lock = ReentrantReadWriteLock(true) - - /* - * Returns true if the database already existed + /** + * Returns true if the database already existed. It is not safe to call this method + * concurrently with [dropAllTablesAndClose] */ internal fun open(driverClass: String, listener: MigrationListener?): Boolean { try { @@ -97,6 +111,11 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc throw DbException(e) } // Open the database and create the tables and indexes if necessary + // First: mark the database as not closed + connectionsLock.withLock { + closed = false + } + // Check if we have the settings table to determine if we're reopening an existing database LOG.info { "checking for settings table" } val reopen = databaseHasSettingsTable() LOG.info { @@ -126,11 +145,8 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc compactAndClose() logDuration(LOG, start) { "Compacting database" } // Allow the next transaction to reopen the DB - connectionsLock.lock() - try { + connectionsLock.withLock { closed = false - } finally { - connectionsLock.unlock() } write { txn -> storeLastCompacted(txn.unbox()) @@ -205,46 +221,40 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc */ private fun startTransaction(readOnly: Boolean): Transaction { // Don't allow reentrant locking - check(lock.readHoldCount <= 0) - check(lock.writeHoldCount <= 0) + check(transactionLock.readHoldCount <= 0) + check(transactionLock.writeHoldCount <= 0) val start = now() if (readOnly) { - lock.readLock().lock() + transactionLock.readLock().lock() logDuration(LOG, start) { "Waiting for read lock" } } else { - lock.writeLock().lock() + transactionLock.writeLock().lock() logDuration(LOG, start) { "Waiting for write lock" } } return try { Transaction(startTransaction(), readOnly) - } catch (e: DbException) { - if (readOnly) lock.readLock().unlock() else lock.writeLock().unlock() - throw e - } catch (e: RuntimeException) { - if (readOnly) lock.readLock().unlock() else lock.writeLock().unlock() + } catch (e: Throwable) { + if (readOnly) { + transactionLock.readLock().unlock() + } else { + transactionLock.writeLock().unlock() + } throw e } } private fun startTransaction(): Connection { - var connection: Connection? - connectionsLock.lock() - connection = try { + var connection = connectionsLock.withLock { if (closed) throw DbClosedException() connections.poll() - } finally { - connectionsLock.unlock() } try { if (connection == null) { // Open a new connection connection = createConnection() connection.autoCommit = false - connectionsLock.lock() - try { + connectionsLock.withLock { openConnections++ - } finally { - connectionsLock.unlock() } } } catch (e: SQLException) { @@ -256,24 +266,18 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc private fun abortTransaction(connection: Connection) { try { connection.rollback() - connectionsLock.lock() - try { + connectionsLock.withLock { connections.add(connection) connectionsChanged.signalAll() - } finally { - connectionsLock.unlock() } } catch (e: SQLException) { // Try to close the connection logException(LOG, e) { "Error while aborting transaction" } tryToClose(connection, LOG) // Whatever happens, allow the database to close - connectionsLock.lock() - try { + connectionsLock.withLock { openConnections-- connectionsChanged.signalAll() - } finally { - connectionsLock.unlock() } } } @@ -284,20 +288,16 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc } catch (e: SQLException) { throw DbException(e) } - connectionsLock.lock() - try { + connectionsLock.withLock { connections.add(connection) connectionsChanged.signalAll() - } finally { - connectionsLock.unlock() } } @Throws(SQLException::class) fun closeAllConnections() { var interrupted = false - connectionsLock.lock() - try { + connectionsLock.withLock { closed = true for (c in connections) c.close() openConnections -= connections.size @@ -313,8 +313,6 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc openConnections -= connections.size connections.clear() } - } finally { - connectionsLock.unlock() } if (interrupted) Thread.currentThread().interrupt() } @@ -388,16 +386,20 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc @Throws(DbException::class) override fun dropAllTablesAndClose() { - connectionsLock.lock() - try { - write { txn -> - val connection: Connection = txn.unbox() - execute(connection, "DROP TABLE settings") - execute(connection, "DROP TABLE contacts") + connectionsLock.withLock { + if (closed) throw DbClosedException() + var c: Connection? = null + try { + c = createConnection() + // Prevent new transactions from starting, wait for any ongoing transactions to finish + closeAllConnections() + execute(c, "DROP TABLE settings") + execute(c, "DROP TABLE contacts") + c.close() + } catch (e: SQLException) { + tryToClose(c, LOG) + throw DbException(e) } - closeAllConnections() - } finally { - connectionsLock.unlock() } } @@ -643,7 +645,11 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc abortTransaction(connection) } } finally { - if (txn.isReadOnly) lock.readLock().unlock() else lock.writeLock().unlock() + if (txn.isReadOnly) { + transactionLock.readLock().unlock() + } else { + transactionLock.writeLock().unlock() + } } } diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt index ffa34af39653988f58da63d9a0d8849287791240..aa9f5454182b86bec474106d88376dbfcdedf349 100644 --- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerIntegrationTest.kt @@ -27,8 +27,8 @@ import kotlin.test.assertEquals class ContactsManagerIntegrationTest : IntegrationTest() { @BeforeEach - override fun initDb() { - super.initDb() + override fun beforeEach() { + super.beforeEach() addOwnerToken() } diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt index 0f7456792197b2a6bd14f3f0ec549b1f4dc470c5..6acd4fa506e2b53d9a21a3cf0cb8a726e9c48b59 100644 --- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/contacts/ContactsManagerMalformedInputIntegrationTest.kt @@ -19,8 +19,8 @@ import kotlin.test.assertEquals class ContactsManagerMalformedInputIntegrationTest : IntegrationTest(false) { @BeforeEach - override fun initDb() { - super.initDb() + override fun beforeEach() { + super.beforeEach() addOwnerToken() } diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt index fd0d67b97374bf1a1ce6a4865ea2cbe3fa801430..cd92398a51cfbb33d007f1e377cbac9b4e55260a 100644 --- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/files/FileManagerIntegrationTest.kt @@ -23,8 +23,8 @@ class FileManagerIntegrationTest : IntegrationTest() { private val bytes = Random.nextBytes(2048) @BeforeEach - override fun initDb() { - super.initDb() + override fun beforeEach() { + super.beforeEach() addOwnerToken() addContact(contact1) addContact(contact2) diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt index bbb9f151b9da66d78110d5b3687f2be17be3807c..778f9ed08e08029b9afff25ad39e886b3fce8929 100644 --- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/IntegrationTest.kt @@ -22,7 +22,10 @@ import org.junit.jupiter.api.BeforeAll import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.TestInstance import org.junit.jupiter.api.TestInstance.Lifecycle +import org.junit.jupiter.api.fail import org.junit.jupiter.api.io.TempDir +import org.slf4j.Logger +import org.slf4j.LoggerFactory import java.io.File import kotlin.test.assertFalse import kotlin.test.assertTrue @@ -30,12 +33,16 @@ import kotlin.test.assertTrue @TestInstance(Lifecycle.PER_CLASS) abstract class IntegrationTest(private val installJsonFeature: Boolean = true) { + companion object { + private val LOG: Logger = LoggerFactory.getLogger(IntegrationTest::class.java) + } + protected lateinit var testComponent: TestComponent protected val db by lazy { testComponent.getDatabase() } private val lifecycleManager by lazy { testComponent.getLifecycleManager() } protected val setupManager by lazy { testComponent.getSetupManager() } protected val metadataManager by lazy { testComponent.getMetadataManager() } - private val wipeManager by lazy { testComponent.getWipeManager() } + protected val wipeManager by lazy { testComponent.getWipeManager() } protected val httpClient = HttpClient(CIO) { expectSuccess = false // prevents exceptions on non-success responses if (installJsonFeature) { @@ -55,6 +62,18 @@ abstract class IntegrationTest(private val installJsonFeature: Boolean = true) { protected val contact1 = getNewRandomContact() protected val contact2 = getNewRandomContact() + @Volatile + protected var exceptionInBackgroundThread: Throwable? = null + + init { + // Ensure exceptions thrown on worker threads cause tests to fail + val fail = Thread.UncaughtExceptionHandler { _: Thread?, throwable: Throwable -> + LOG.warn("Caught unhandled exception", throwable) + exceptionInBackgroundThread = throwable + } + Thread.setDefaultUncaughtExceptionHandler(fail) + } + @BeforeAll fun setUp(@TempDir tempDir: File) { testComponent = DaggerTestComponent.builder().testModule(TestModule(tempDir)).build() @@ -71,7 +90,8 @@ abstract class IntegrationTest(private val installJsonFeature: Boolean = true) { } @BeforeEach - open fun initDb() { + open fun beforeEach() { + exceptionInBackgroundThread = null // need to reopen database here because we're closing it after each test db.open(null) db.read { txn -> @@ -82,9 +102,22 @@ abstract class IntegrationTest(private val installJsonFeature: Boolean = true) { } @AfterEach - open fun wipe() { - wipeManager.wipeDatabaseAndFiles() - assertFalse(setupManager.hasDb) + open fun afterEach() { + afterEach(true) + } + + fun afterEach(wipe: Boolean) { + if (wipe) { + wipeManager.wipeDatabaseAndFiles() + assertFalse(setupManager.hasDb) + } + + if (exceptionInBackgroundThread != null) { + fail( + "background thread has thrown an exception unexpectedly", + exceptionInBackgroundThread + ) + } } protected fun addOwnerToken() { diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/ThreadExceptionTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/ThreadExceptionTest.kt new file mode 100644 index 0000000000000000000000000000000000000000..4a1a570c1c8c5c1d272b68ed771de6674d6b10db --- /dev/null +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/server/ThreadExceptionTest.kt @@ -0,0 +1,20 @@ +package org.briarproject.mailbox.core.server + +import org.junit.jupiter.api.Test +import kotlin.concurrent.thread +import kotlin.test.assertNotNull + +class ThreadExceptionTest : IntegrationTest() { + + @Test + fun `exception thrown on background thread gets caught and stored`() { + val t = thread(name = "failing thread") { + throw RuntimeException("boom") + } + t.join() + + assertNotNull(exceptionInBackgroundThread) + exceptionInBackgroundThread = null + } + +} diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt index 726a6f8cbf6cd9ce343b04c04cdc60e5e64de7d4..71fa450b14929c1836a3fe0bab9e5c395f2f776c 100644 --- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/settings/MetadataRouteManagerTest.kt @@ -13,8 +13,8 @@ import kotlin.test.assertEquals class MetadataRouteManagerTest : IntegrationTest() { @BeforeEach - override fun initDb() { - super.initDb() + override fun beforeEach() { + super.beforeEach() addOwnerToken() addContact(contact1) addContact(contact2) diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt new file mode 100644 index 0000000000000000000000000000000000000000..e3c622e824e628d275c7211c3ef61964465652a0 --- /dev/null +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt @@ -0,0 +1,35 @@ +package org.briarproject.mailbox.core.setup + +import org.briarproject.mailbox.core.server.IntegrationTest +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Test +import kotlin.concurrent.thread + +class WipeTest1 : IntegrationTest() { + + @Test + fun `wiping and concurrent database access doesn't cause deadlocks`() { + val t1 = thread(name = "dropper") { + wipeManager.wipeDatabaseAndFiles() + } + // This most likely throws a DbException within the thread, but if it does is doesn't make + // the test fail. + val t2 = thread(name = "other") { + addOwnerToken() + } + t1.join() + t2.join() + + // reset field for exceptions thrown on background threads as this can indeed happen here + // and is OK + exceptionInBackgroundThread = null + } + + @AfterEach + override fun afterEach() { + // We need to pass false here because calling wipeDatabaseAndFiles() with a closed Database + // throws a DbClosedException. + afterEach(false) + } + +} diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt new file mode 100644 index 0000000000000000000000000000000000000000..dd2256c157c42b63f4318f040f4652f36168b422 --- /dev/null +++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt @@ -0,0 +1,66 @@ +package org.briarproject.mailbox.core.setup + +import org.briarproject.mailbox.core.server.IntegrationTest +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Test +import kotlin.concurrent.thread +import kotlin.test.assertEquals + +class WipeTest2 : IntegrationTest() { + + var failed = 0 + var succeeded = 0 + + @Synchronized + fun incrementFailure() { + failed += 1 + } + + @Synchronized + fun incrementSuccess() { + succeeded += 1 + } + + @Test + fun `concurrent wipe requests don't interfere and database access doesn't cause deadlocks`() { + // One of the two dropper threads will succeed, the other will fail. + val t1 = thread(name = "dropper 1") { + try { + wipeManager.wipeDatabaseAndFiles() + incrementSuccess() + } catch (t: Throwable) { + incrementFailure() + } + } + val t2 = thread(name = "dropper 2") { + try { + wipeManager.wipeDatabaseAndFiles() + incrementSuccess() + } catch (t: Throwable) { + incrementFailure() + } + } + // This most likely throws a DbException within the thread, but if it does is doesn't make + // the test fail. + val t3 = thread(name = "other") { + addOwnerToken() + } + t1.join() + t2.join() + t3.join() + assertEquals(1, succeeded) + assertEquals(1, failed) + + // reset field for exceptions thrown on background threads as this can indeed happen here + // and is OK + exceptionInBackgroundThread = null + } + + @AfterEach + override fun afterEach() { + // We need to pass false here because calling wipeDatabaseAndFiles() with a closed Database + // throws a DbClosedException. + afterEach(false) + } + +}