From 64f682146ddb901209850eab8118c0d1fb347627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20K=C3=BCrten?= Date: Mon, 12 Apr 2021 12:23:15 +0200 Subject: [PATCH] Integrate merge request feedback --- .../briarproject/bramble/db/H2Database.java | 1 - .../bramble/db/HyperSqlDatabase.java | 1 - .../briarproject/bramble/db/JdbcDatabase.java | 4 +- .../bramble/db/JdbcDatabaseTest.java | 65 ++++++++++--------- 4 files changed, 37 insertions(+), 34 deletions(-) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/H2Database.java b/bramble-core/src/main/java/org/briarproject/bramble/db/H2Database.java index 89d0c0152..ce4433005 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/H2Database.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/H2Database.java @@ -89,7 +89,6 @@ class H2Database extends JdbcDatabase { c = createConnection(); super.closeAllConnections(); setDirty(c, false); - c.commit(); c.close(); } catch (SQLException e) { tryToClose(c, LOG, WARNING); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/HyperSqlDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/HyperSqlDatabase.java index 6aa09a6ee..bdc985ed0 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/HyperSqlDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/HyperSqlDatabase.java @@ -82,7 +82,6 @@ class HyperSqlDatabase extends JdbcDatabase { super.closeAllConnections(); c = createConnection(); setDirty(c, false); - c.commit(); s = c.createStatement(); s.executeQuery("SHUTDOWN"); s.close(); diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java index 62fde5c20..b559b2924 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/JdbcDatabase.java @@ -355,13 +355,13 @@ abstract class JdbcDatabase implements Database { @GuardedBy("connectionsLock") private boolean closed = false; - private boolean wasDirtyOnInitialisation = false; + private volatile boolean wasDirtyOnInitialisation = false; protected abstract Connection createConnection() throws DbException, SQLException; // Used exclusively during open to compact the database after schema - // migrations and after DatabaseConstants#MAX_COMPACTION_INTERVAL_MS has + // migrations or after DatabaseConstants#MAX_COMPACTION_INTERVAL_MS has // elapsed protected abstract void compactAndClose() throws DbException; diff --git a/bramble-core/src/test/java/org/briarproject/bramble/db/JdbcDatabaseTest.java b/bramble-core/src/test/java/org/briarproject/bramble/db/JdbcDatabaseTest.java index 81e832ff5..40d9b7059 100644 --- a/bramble-core/src/test/java/org/briarproject/bramble/db/JdbcDatabaseTest.java +++ b/bramble-core/src/test/java/org/briarproject/bramble/db/JdbcDatabaseTest.java @@ -2354,37 +2354,8 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase { @Test public void testShutdownGracefully() throws Exception { - CountDownLatch closing = new CountDownLatch(1); - CountDownLatch closed = new CountDownLatch(1); - AtomicBoolean transactionFinished = new AtomicBoolean(false); - AtomicBoolean error = new AtomicBoolean(false); Database db = open(false); - - // Start a transaction - Connection txn = db.startTransaction(); - // In another thread, close the database - Thread close = new Thread(() -> { - try { - closing.countDown(); - db.close(); - if (!transactionFinished.get()) error.set(true); - closed.countDown(); - } catch (Exception e) { - error.set(true); - } - }); - close.start(); - closing.await(); - // Do whatever the transaction needs to do - Thread.sleep(10); - transactionFinished.set(true); - // Abort the transaction - db.abortTransaction(txn); - // The other thread should now terminate - assertTrue(closed.await(5, SECONDS)); - // Check that the other thread didn't encounter an error - assertFalse(error.get()); - + db.close(); open(true); assertFalse(db.wasDirtyOnInitialisation()); } @@ -2393,6 +2364,35 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase { public void testShutdownDirty() throws Exception { Database db = open(false); + // We want to simulate a dirty shutdown here which would normally be + // caused by an empty battery or by force closing the Android app. + // As there is no obvious way to simulate this, we're artificially + // causing an SqlException during close() here by unloading the JDBC + // drivers. + List unloadedDrivers = unloadDrivers(); + + try { + db.close(); + fail(); + } catch (Exception e) { + // continue + e.printStackTrace(); + } + + // Reloading drivers to continue so that we're able to work with the + // database again. + reloadDrivers(unloadedDrivers); + + db = open(true); + assertTrue(db.wasDirtyOnInitialisation()); + } + + @Test + public void testShutdownDirtyThenGracefully() throws Exception { + Database db = open(false); + + // Simulating a dirty shutdown here, look at #testShutdownDirty for + // details. List unloadedDrivers = unloadDrivers(); try { @@ -2406,6 +2406,10 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase { db = open(true); assertTrue(db.wasDirtyOnInitialisation()); + + db.close(); + db = open(true); + assertFalse(db.wasDirtyOnInitialisation()); } private Database open(boolean resume) throws Exception { @@ -2473,6 +2477,7 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase { unloaded.add(d.getClass().getName()); } catch (SQLException e) { e.printStackTrace(); + fail(); } } return unloaded;