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..df4aebcc82750f7aa016169fa702a15a6e61810d 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,41 @@ 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()
+ if (readOnly) transactionLock.readLock().unlock() else transactionLock.writeLock()
+ .unlock()
throw e
} catch (e: RuntimeException) {
- if (readOnly) lock.readLock().unlock() else lock.writeLock().unlock()
+ 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 +267,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 +289,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 +314,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 +387,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 +646,8 @@ 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/setup/WipeTest1.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt
new file mode 100644
index 0000000000000000000000000000000000000000..e140ae19892692057770cc517bc2df8d397e7f37
--- /dev/null
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest1.kt
@@ -0,0 +1,31 @@
+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 test() {
+ val t1 = thread(name = "dropper") {
+ db.dropAllTablesAndClose()
+ }
+ // 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()
+ }
+
+ @AfterEach
+ override fun clearDb() {
+ // This is not expected to work because calling dropAllTablesAndClose() on a closed Database
+ // throws a DbClosedException.
+ // super.clearDb()
+ }
+
+}
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..d7e0ae452d9eb0f837a310c702497de2e49daba3
--- /dev/null
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/setup/WipeTest2.kt
@@ -0,0 +1,62 @@
+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 test() {
+ // One of the two dropper threads will succeed, the other will fail.
+ val t1 = thread(name = "dropper 1") {
+ try {
+ db.dropAllTablesAndClose()
+ incrementSuccess()
+ } catch (t: Throwable) {
+ incrementFailure()
+ }
+ }
+ val t2 = thread(name = "dropper 2") {
+ try {
+ db.dropAllTablesAndClose()
+ 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)
+ }
+
+ @AfterEach
+ override fun clearDb() {
+ // This is not expected to work because calling dropAllTablesAndClose() on a closed Database
+ // throws a DbClosedException.
+ // super.clearDb()
+ }
+
+}