From 9caf115bc7d2f051ce4dd6cde71f5ad5ae86c916 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20K=C3=BCrten?= <sebastian@mobanisto.de>
Date: Wed, 6 Oct 2021 12:59:37 +0200
Subject: [PATCH] Address review feedback
---
gradle.properties | 3 +-
.../mailbox/core/db/JdbcDatabase.kt | 42 +++++++-----
.../mailbox/core/db/Transaction.kt | 6 +-
.../mailbox/core/db/TransactionManager.kt | 25 -------
.../core/lifecycle/LifecycleManager.java | 8 ++-
.../mailbox/core/db/JdbcDatabaseTest.kt | 65 +++++++++----------
6 files changed, 66 insertions(+), 83 deletions(-)
diff --git a/gradle.properties b/gradle.properties
index 2d8d1e4d..98026c56 100644
--- a/gradle.properties
+++ b/gradle.properties
@@ -1 +1,2 @@
-android.useAndroidX=true
\ No newline at end of file
+android.useAndroidX=true
+org.gradle.jvmargs=-Xmx1g
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 79fafc22..b46c2d34 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
@@ -75,9 +75,9 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc
throw DbException(e)
}
// Open the database and create the tables and indexes if necessary
- val compact: Boolean
- var connection = startTransaction()
- try {
+ var compact = false
+ transaction(false) { txn ->
+ val connection = txn.unbox()
compact = if (reopen) {
val s: Settings = getSettings(connection, DB_SETTINGS_NAMESPACE)
wasDirtyOnInitialisation = isDirty(s)
@@ -92,10 +92,6 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc
LOG.info("db dirty? $wasDirtyOnInitialisation")
}
createIndexes(connection)
- commitTransaction(connection)
- } catch (e: DbException) {
- abortTransaction(connection)
- throw e
}
// Compact the database if necessary
if (compact) {
@@ -105,13 +101,8 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc
logDuration(LOG, { "Compacting database" }, start)
// Allow the next transaction to reopen the DB
synchronized(connectionsLock) { closed = false }
- connection = startTransaction()
- try {
- storeLastCompacted(connection)
- commitTransaction(connection)
- } catch (e: DbException) {
- abortTransaction(connection)
- throw e
+ transaction(false) { txn ->
+ storeLastCompacted(txn.unbox())
}
}
}
@@ -169,7 +160,15 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc
@Throws(DbException::class)
protected abstract fun compactAndClose()
- override fun startTransaction(readOnly: Boolean): Transaction {
+ /**
+ * Starts a new transaction and returns an object representing it.
+ *
+ * This method acquires locks, so it must not be called while holding a
+ * lock.
+ *
+ * @param readOnly true if the transaction will only be used for reading.
+ */
+ private fun startTransaction(readOnly: Boolean): Transaction {
// Don't allow reentrant locking
check(lock.readHoldCount <= 0)
check(lock.writeHoldCount <= 0)
@@ -506,15 +505,24 @@ abstract class JdbcDatabase(private val dbTypes: DatabaseTypes, private val cloc
}
}
+ /**
+ * Commits a transaction to the database.
+ */
@Throws(DbException::class)
- override fun commitTransaction(txn: Transaction) {
+ private fun commitTransaction(txn: Transaction) {
val connection: Connection = txn.unbox() as Connection
check(!txn.isCommitted)
txn.setCommitted()
commitTransaction(connection)
}
- override fun endTransaction(txn: Transaction) {
+ /**
+ * Ends a transaction. If the transaction has not been committed,
+ * it will be aborted. If the transaction has been committed,
+ * any events attached to the transaction are broadcast.
+ * The database lock will be released in either case.
+ */
+ private fun endTransaction(txn: Transaction) {
try {
val connection: Connection = txn.unbox() as Connection
if (!txn.isCommitted) {
diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/Transaction.kt b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/Transaction.kt
index 92283126..81176661 100644
--- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/Transaction.kt
+++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/Transaction.kt
@@ -1,7 +1,9 @@
package org.briarproject.mailbox.core.db
+import java.sql.Connection
+
class Transaction(
- private val txn: Any,
+ private val txn: Connection,
/**
* Returns true if the transaction can only be used for reading.
*/
@@ -18,7 +20,7 @@ class Transaction(
* Returns the database transaction. The type of the returned object
* depends on the database implementation.
*/
- fun unbox(): Any {
+ fun unbox(): Connection {
return txn
}
diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/TransactionManager.kt b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/TransactionManager.kt
index 8df28a72..d663509b 100644
--- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/TransactionManager.kt
+++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/db/TransactionManager.kt
@@ -1,31 +1,6 @@
package org.briarproject.mailbox.core.db
interface TransactionManager {
- /**
- * Starts a new transaction and returns an object representing it.
- *
- *
- * This method acquires locks, so it must not be called while holding a
- * lock.
- *
- * @param readOnly true if the transaction will only be used for reading.
- */
- @Throws(DbException::class)
- fun startTransaction(readOnly: Boolean): Transaction
-
- /**
- * Commits a transaction to the database.
- */
- @Throws(DbException::class)
- fun commitTransaction(txn: Transaction)
-
- /**
- * Ends a transaction. If the transaction has not been committed,
- * it will be aborted. If the transaction has been committed,
- * any events attached to the transaction are broadcast.
- * The database lock will be released in either case.
- */
- fun endTransaction(txn: Transaction)
/**
* Runs the given task within a transaction.
diff --git a/mailbox-core/src/main/java/org/briarproject/mailbox/core/lifecycle/LifecycleManager.java b/mailbox-core/src/main/java/org/briarproject/mailbox/core/lifecycle/LifecycleManager.java
index 84c09ac3..2e8aa3ee 100644
--- a/mailbox-core/src/main/java/org/briarproject/mailbox/core/lifecycle/LifecycleManager.java
+++ b/mailbox-core/src/main/java/org/briarproject/mailbox/core/lifecycle/LifecycleManager.java
@@ -29,9 +29,13 @@ public interface LifecycleManager {
*/
enum LifecycleState {
- STOPPED, STARTING, MIGRATING_DATABASE, COMPACTING_DATABASE,
+ STOPPED,
+ STARTING,
+ MIGRATING_DATABASE,
+ COMPACTING_DATABASE,
STARTING_SERVICES,
- RUNNING, STOPPING;
+ RUNNING,
+ STOPPING;
public boolean isAfter(LifecycleState state) {
return ordinal() > state.ordinal();
diff --git a/mailbox-core/src/test/java/org/briarproject/mailbox/core/db/JdbcDatabaseTest.kt b/mailbox-core/src/test/java/org/briarproject/mailbox/core/db/JdbcDatabaseTest.kt
index 9fe113ef..b711dcb0 100644
--- a/mailbox-core/src/test/java/org/briarproject/mailbox/core/db/JdbcDatabaseTest.kt
+++ b/mailbox-core/src/test/java/org/briarproject/mailbox/core/db/JdbcDatabaseTest.kt
@@ -36,9 +36,6 @@ abstract class JdbcDatabaseTest {
@Throws(Exception::class)
open fun testPersistence() {
// Store some records
- var db: Database = open(false)
- var txn = db.startTransaction(false)
-
val contact1 = Contact(
1,
"4291ad1d-897d-4db4-9de9-ea3f78c5262e",
@@ -51,37 +48,35 @@ abstract class JdbcDatabaseTest {
"7931fa7a-077e-403a-8487-63261027d6d2",
"12a61ca3-af0a-41d1-acc1-a0f4625f6e42"
)
+ var db: Database = open(false)
+ db.transaction(false) { txn ->
- db.addContact(txn, contact1)
- db.addContact(txn, contact2)
-
- db.commitTransaction(txn)
+ db.addContact(txn, contact1)
+ db.addContact(txn, contact2)
+ }
db.close()
// Check that the records are still there
db = open(true)
- txn = db.startTransaction(false)
- val contact1Reloaded1 = db.getContact(txn, 1)
- val contact2Reloaded1 = db.getContact(txn, 2)
- assertEquals(contact1, contact1Reloaded1)
- assertEquals(contact2, contact2Reloaded1)
-
- // Delete one of the records
- db.removeContact(txn, 1)
-
- db.commitTransaction(txn)
+ db.transaction(false) { txn ->
+ val contact1Reloaded1 = db.getContact(txn, 1)
+ val contact2Reloaded1 = db.getContact(txn, 2)
+ assertEquals(contact1, contact1Reloaded1)
+ assertEquals(contact2, contact2Reloaded1)
+
+ // Delete one of the records
+ db.removeContact(txn, 1)
+ }
db.close()
// Check that the record is gone
db = open(true)
- txn = db.startTransaction(true)
-
- val contact1Reloaded2 = db.getContact(txn, 1)
- val contact2Reloaded2 = db.getContact(txn, 2)
- assertNull(contact1Reloaded2)
- assertEquals(contact2, contact2Reloaded2)
-
- db.commitTransaction(txn)
+ db.transaction(true) { txn ->
+ val contact1Reloaded2 = db.getContact(txn, 1)
+ val contact2Reloaded2 = db.getContact(txn, 2)
+ assertNull(contact1Reloaded2)
+ assertEquals(contact2, contact2Reloaded2)
+ }
db.close()
}
@@ -98,17 +93,15 @@ abstract class JdbcDatabaseTest {
merged["baz"] = "qux"
var db: Database = open(false)
- var txn = db.startTransaction(false)
-
- // store 'before'
- db.mergeSettings(txn, before, "namespace")
- assertEquals(before, db.getSettings(txn, "namespace"))
-
- // merge 'update'
- db.mergeSettings(txn, update, "namespace")
- assertEquals(merged, db.getSettings(txn, "namespace"))
-
- db.commitTransaction(txn)
+ var txn = db.transaction(false) { txn ->
+ // store 'before'
+ db.mergeSettings(txn, before, "namespace")
+ assertEquals(before, db.getSettings(txn, "namespace"))
+
+ // merge 'update'
+ db.mergeSettings(txn, update, "namespace")
+ assertEquals(merged, db.getSettings(txn, "namespace"))
+ }
db.close()
}
--
GitLab