From db84d07c3808f37b59d01d1824d644a190322104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20K=C3=BCrten?= Date: Tue, 6 Apr 2021 12:15:38 +0200 Subject: [PATCH 1/4] Store a dirty flag in the database --- .../org/briarproject/bramble/db/Database.java | 7 ++++++ .../bramble/db/DatabaseConstants.java | 6 +++++ .../briarproject/bramble/db/H2Database.java | 6 +++++ .../bramble/db/HyperSqlDatabase.java | 2 ++ .../briarproject/bramble/db/JdbcDatabase.java | 24 +++++++++++++++++++ 5 files changed, 45 insertions(+) diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java b/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java index f6745af24..a256d91ff 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/Database.java @@ -68,6 +68,13 @@ interface Database { */ void close() throws DbException; + /** + * Returns true if the dirty flag was set while opening the database, + * indicating that the database has not been shut down properly the last + * time it was closed and some data could be lost. + */ + boolean wasDirtyOnInitialisation(); + /** * Starts a new transaction and returns an object representing it. */ diff --git a/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java b/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java index f27d45ced..11e4ba64c 100644 --- a/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java +++ b/bramble-core/src/main/java/org/briarproject/bramble/db/DatabaseConstants.java @@ -37,4 +37,10 @@ interface DatabaseConstants { * has passed since the last compaction. */ long MAX_COMPACTION_INTERVAL_MS = DAYS.toMillis(30); + + /** + * The {@link Settings} key under which the flag is stored indicating + * whether the database is marked as dirty. + */ + String DIRTY_KEY = "dirty"; } 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 fe59c35a1..89d0c0152 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 @@ -84,9 +84,15 @@ class H2Database extends JdbcDatabase { @Override public void close() throws DbException { // H2 will close the database when the last connection closes + Connection c = null; try { + c = createConnection(); super.closeAllConnections(); + setDirty(c, false); + c.commit(); + c.close(); } catch (SQLException e) { + tryToClose(c, LOG, WARNING); throw new DbException(e); } } 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 aad359498..6aa09a6ee 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 @@ -81,6 +81,8 @@ class HyperSqlDatabase extends JdbcDatabase { try { 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 c56b48e8d..4f7fe69d9 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 @@ -81,6 +81,7 @@ import static org.briarproject.bramble.api.sync.validation.MessageState.DELIVERE import static org.briarproject.bramble.api.sync.validation.MessageState.PENDING; import static org.briarproject.bramble.api.sync.validation.MessageState.UNKNOWN; import static org.briarproject.bramble.db.DatabaseConstants.DB_SETTINGS_NAMESPACE; +import static org.briarproject.bramble.db.DatabaseConstants.DIRTY_KEY; import static org.briarproject.bramble.db.DatabaseConstants.LAST_COMPACTED_KEY; import static org.briarproject.bramble.db.DatabaseConstants.MAX_COMPACTION_INTERVAL_MS; import static org.briarproject.bramble.db.DatabaseConstants.SCHEMA_VERSION_KEY; @@ -354,9 +355,14 @@ abstract class JdbcDatabase implements Database { @GuardedBy("connectionsLock") private boolean closed = false; + private 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 + // elapsed protected abstract void compactAndClose() throws DbException; JdbcDatabase(DatabaseTypes databaseTypes, MessageFactory messageFactory, @@ -381,13 +387,16 @@ abstract class JdbcDatabase implements Database { try { if (reopen) { Settings s = getSettings(txn, DB_SETTINGS_NAMESPACE); + wasDirtyOnInitialisation = isDirty(s); compact = migrateSchema(txn, s, listener) || isCompactionDue(s); } else { + wasDirtyOnInitialisation = false; createTables(txn); initialiseSettings(txn); compact = false; } createIndexes(txn); + setDirty(txn, true); commitTransaction(txn); } catch (DbException e) { abortTransaction(txn); @@ -414,6 +423,11 @@ abstract class JdbcDatabase implements Database { } } + @Override + public boolean wasDirtyOnInitialisation() { + return wasDirtyOnInitialisation; + } + /** * Compares the schema version stored in the database with the schema * version used by the current code and applies any suitable migrations to @@ -488,6 +502,16 @@ abstract class JdbcDatabase implements Database { mergeSettings(txn, s, DB_SETTINGS_NAMESPACE); } + private boolean isDirty(Settings s) { + return s.getBoolean(DIRTY_KEY, false); + } + + protected void setDirty(Connection txn, boolean dirty) throws DbException { + Settings s = new Settings(); + s.putBoolean(DIRTY_KEY, dirty); + mergeSettings(txn, s, DB_SETTINGS_NAMESPACE); + } + private void initialiseSettings(Connection txn) throws DbException { Settings s = new Settings(); s.putInt(SCHEMA_VERSION_KEY, CODE_SCHEMA_VERSION); From e99df2b69e9b40c4fb6964d55ce209c7b74d9e3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20K=C3=BCrten?= Date: Tue, 6 Apr 2021 20:19:28 +0200 Subject: [PATCH 2/4] Log dirty flag when opening database --- .../main/java/org/briarproject/bramble/db/JdbcDatabase.java | 3 +++ 1 file changed, 3 insertions(+) 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 4f7fe69d9..62fde5c20 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 @@ -395,6 +395,9 @@ abstract class JdbcDatabase implements Database { initialiseSettings(txn); compact = false; } + if (LOG.isLoggable(INFO)) { + LOG.info("db dirty? " + wasDirtyOnInitialisation); + } createIndexes(txn); setDirty(txn, true); commitTransaction(txn); From ce47bfe01861ab709f8fdac84434fd7cbc37dcfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20K=C3=BCrten?= Date: Tue, 6 Apr 2021 20:39:59 +0200 Subject: [PATCH 3/4] Write tests for the dirty flag --- .../bramble/db/JdbcDatabaseTest.java | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) 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 ac4231216..81e832ff5 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 @@ -41,8 +41,12 @@ import org.junit.Test; import java.io.File; import java.sql.Connection; +import java.sql.Driver; +import java.sql.DriverManager; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Collection; +import java.util.Enumeration; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -2347,6 +2351,63 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase { db.close(); } + + @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()); + + open(true); + assertFalse(db.wasDirtyOnInitialisation()); + } + + @Test + public void testShutdownDirty() throws Exception { + Database db = open(false); + + List unloadedDrivers = unloadDrivers(); + + try { + db.close(); + fail(); + } catch (Exception e) { + // continue + } + + reloadDrivers(unloadedDrivers); + + db = open(true); + assertTrue(db.wasDirtyOnInitialisation()); + } + private Database open(boolean resume) throws Exception { return open(resume, new TestMessageFactory(), new SystemClock()); } @@ -2402,6 +2463,30 @@ public abstract class JdbcDatabaseTest extends BrambleTestCase { rootKey, alice); } + private List unloadDrivers() { + Enumeration drivers = DriverManager.getDrivers(); + List unloaded = new ArrayList<>(); + while (drivers.hasMoreElements()) { + Driver d = drivers.nextElement(); + try { + DriverManager.deregisterDriver(d); + unloaded.add(d.getClass().getName()); + } catch (SQLException e) { + e.printStackTrace(); + } + } + return unloaded; + } + + private void reloadDrivers(List unloadedDrivers) + throws ClassNotFoundException, IllegalAccessException, + InstantiationException, SQLException { + for (String driverName : unloadedDrivers) { + DriverManager.registerDriver( + (Driver) Class.forName(driverName).newInstance()); + } + } + @After public void tearDown() { deleteTestDirectory(testDir); 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 4/4] 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;