From 8ead3720c09bcaee11680072355c4e4dfab939a2 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 12:50:42 -0500 Subject: [PATCH 1/5] Move as much work as possible out of the sqlite txn In particular, don't do anything that could throw in the finally block, as this will mask any exception thrown within the transaction itself. --- .../firestore/local/SQLitePersistence.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index 891aa091729..ea86ff2f9be 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -143,35 +143,36 @@ RemoteDocumentCache getRemoteDocumentCache() { @Override void runTransaction(String action, Runnable operation) { + Logger.debug(TAG, "Starting transaction: %s", action); + referenceDelegate.onTransactionStarted(); + db.beginTransaction(); try { - Logger.debug(TAG, "Starting transaction: %s", action); - referenceDelegate.onTransactionStarted(); - db.beginTransaction(); operation.run(); // Note that an exception in operation.run() will prevent this code from running. db.setTransactionSuccessful(); } finally { db.endTransaction(); - referenceDelegate.onTransactionCommitted(); } + referenceDelegate.onTransactionCommitted(); } @Override T runTransaction(String action, Supplier operation) { + Logger.debug(TAG, "Starting transaction: %s", action); + T value = null; + referenceDelegate.onTransactionStarted(); + db.beginTransaction(); try { - Logger.debug(TAG, "Starting transaction: %s", action); - referenceDelegate.onTransactionStarted(); - db.beginTransaction(); - T value = operation.get(); + value = operation.get(); // Note that an exception in operation.run() will prevent this code from running. db.setTransactionSuccessful(); - return value; } finally { db.endTransaction(); - referenceDelegate.onTransactionCommitted(); } + referenceDelegate.onTransactionCommitted(); + return value; } /** From 4a9dcd1baef663c4f17fd1ce1f175716a3e30780 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 13:01:41 -0500 Subject: [PATCH 2/5] Use sqlite transaction listeners (rather than doing so manually) Using sqlite's version allows us to not worry about getting the transaction semantics correct in the face of exceptions. --- .../local/SQLiteLruReferenceDelegate.java | 17 ++++++++++++++++- .../firestore/local/SQLitePersistence.java | 8 ++------ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java index cee90c741cc..f762c7531c7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; +import android.database.sqlite.SQLiteTransactionListener; import android.util.SparseArray; import com.google.firebase.firestore.core.ListenSequence; import com.google.firebase.firestore.model.DocumentKey; @@ -23,7 +24,8 @@ import com.google.firebase.firestore.util.Consumer; /** Provides LRU functionality for SQLite persistence. */ -class SQLiteLruReferenceDelegate implements ReferenceDelegate, LruDelegate { +class SQLiteLruReferenceDelegate + implements ReferenceDelegate, LruDelegate, SQLiteTransactionListener { private final SQLitePersistence persistence; private ListenSequence listenSequence; private long currentSequenceNumber; @@ -40,6 +42,19 @@ void start(long highestSequenceNumber) { listenSequence = new ListenSequence(highestSequenceNumber); } + @Override + public void onBegin() { + onTransactionStarted(); + } + + @Override + public void onCommit() { + onTransactionCommitted(); + } + + @Override + public void onRollback() {} + @Override public void onTransactionStarted() { hardAssert( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index ea86ff2f9be..4faaa039f5d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -144,8 +144,7 @@ RemoteDocumentCache getRemoteDocumentCache() { @Override void runTransaction(String action, Runnable operation) { Logger.debug(TAG, "Starting transaction: %s", action); - referenceDelegate.onTransactionStarted(); - db.beginTransaction(); + db.beginTransactionWithListener(referenceDelegate); try { operation.run(); @@ -154,15 +153,13 @@ void runTransaction(String action, Runnable operation) { } finally { db.endTransaction(); } - referenceDelegate.onTransactionCommitted(); } @Override T runTransaction(String action, Supplier operation) { Logger.debug(TAG, "Starting transaction: %s", action); T value = null; - referenceDelegate.onTransactionStarted(); - db.beginTransaction(); + db.beginTransactionWithListener(referenceDelegate); try { value = operation.get(); @@ -171,7 +168,6 @@ T runTransaction(String action, Supplier operation) { } finally { db.endTransaction(); } - referenceDelegate.onTransactionCommitted(); return value; } From 496e614eab3f70b576b64565e9d1ccdea6e8a4d1 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 13:46:39 -0500 Subject: [PATCH 3/5] Dont implement SQLiteTransactionListener within SQLiteLruReferenceDelegate --- .../local/SQLiteLruReferenceDelegate.java | 17 +--------- .../firestore/local/SQLitePersistence.java | 33 +++++++++++++++++-- 2 files changed, 32 insertions(+), 18 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java index f762c7531c7..cee90c741cc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java @@ -16,7 +16,6 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; -import android.database.sqlite.SQLiteTransactionListener; import android.util.SparseArray; import com.google.firebase.firestore.core.ListenSequence; import com.google.firebase.firestore.model.DocumentKey; @@ -24,8 +23,7 @@ import com.google.firebase.firestore.util.Consumer; /** Provides LRU functionality for SQLite persistence. */ -class SQLiteLruReferenceDelegate - implements ReferenceDelegate, LruDelegate, SQLiteTransactionListener { +class SQLiteLruReferenceDelegate implements ReferenceDelegate, LruDelegate { private final SQLitePersistence persistence; private ListenSequence listenSequence; private long currentSequenceNumber; @@ -42,19 +40,6 @@ void start(long highestSequenceNumber) { listenSequence = new ListenSequence(highestSequenceNumber); } - @Override - public void onBegin() { - onTransactionStarted(); - } - - @Override - public void onCommit() { - onTransactionCommitted(); - } - - @Override - public void onRollback() {} - @Override public void onTransactionStarted() { hardAssert( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index 4faaa039f5d..3547452dacd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -26,6 +26,7 @@ import android.database.sqlite.SQLiteOpenHelper; import android.database.sqlite.SQLiteProgram; import android.database.sqlite.SQLiteStatement; +import android.database.sqlite.SQLiteTransactionListener; import android.support.annotation.VisibleForTesting; import com.google.common.base.Function; import com.google.firebase.firestore.auth.User; @@ -144,7 +145,21 @@ RemoteDocumentCache getRemoteDocumentCache() { @Override void runTransaction(String action, Runnable operation) { Logger.debug(TAG, "Starting transaction: %s", action); - db.beginTransactionWithListener(referenceDelegate); + db.beginTransactionWithListener( + new SQLiteTransactionListener() { + @Override + public void onBegin() { + referenceDelegate.onTransactionStarted(); + } + + @Override + public void onCommit() { + referenceDelegate.onTransactionCommitted(); + } + + @Override + public void onRollback() {} + }); try { operation.run(); @@ -159,7 +174,21 @@ void runTransaction(String action, Runnable operation) { T runTransaction(String action, Supplier operation) { Logger.debug(TAG, "Starting transaction: %s", action); T value = null; - db.beginTransactionWithListener(referenceDelegate); + db.beginTransactionWithListener( + new SQLiteTransactionListener() { + @Override + public void onBegin() { + referenceDelegate.onTransactionStarted(); + } + + @Override + public void onCommit() { + referenceDelegate.onTransactionCommitted(); + } + + @Override + public void onRollback() {} + }); try { value = operation.get(); From 2931f24093352674b4bca2664dd2c77c8d261914 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 13:57:50 -0500 Subject: [PATCH 4/5] Create adapter to reduce duplication And to remove clutter around the sqlite transaction. --- .../firestore/local/SQLitePersistence.java | 52 +++++++++---------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index 3547452dacd..1a0d3a6d103 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -142,24 +142,33 @@ RemoteDocumentCache getRemoteDocumentCache() { return remoteDocumentCache; } + private static class ReferenceDelegateToSQLiteTransactionListenerAdapter + implements SQLiteTransactionListener { + private ReferenceDelegate instance; + + ReferenceDelegateToSQLiteTransactionListenerAdapter(ReferenceDelegate instance) { + this.instance = instance; + } + + @Override + public void onBegin() { + instance.onTransactionStarted(); + } + + @Override + public void onCommit() { + instance.onTransactionCommitted(); + } + + @Override + public void onRollback() {} + } + @Override void runTransaction(String action, Runnable operation) { Logger.debug(TAG, "Starting transaction: %s", action); db.beginTransactionWithListener( - new SQLiteTransactionListener() { - @Override - public void onBegin() { - referenceDelegate.onTransactionStarted(); - } - - @Override - public void onCommit() { - referenceDelegate.onTransactionCommitted(); - } - - @Override - public void onRollback() {} - }); + new ReferenceDelegateToSQLiteTransactionListenerAdapter(referenceDelegate)); try { operation.run(); @@ -175,20 +184,7 @@ T runTransaction(String action, Supplier operation) { Logger.debug(TAG, "Starting transaction: %s", action); T value = null; db.beginTransactionWithListener( - new SQLiteTransactionListener() { - @Override - public void onBegin() { - referenceDelegate.onTransactionStarted(); - } - - @Override - public void onCommit() { - referenceDelegate.onTransactionCommitted(); - } - - @Override - public void onRollback() {} - }); + new ReferenceDelegateToSQLiteTransactionListenerAdapter(referenceDelegate)); try { value = operation.get(); From b2146b95546321940db13b05c4090b7501c869c4 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 21 Nov 2018 16:12:15 -0500 Subject: [PATCH 5/5] Remove adapter in favour of an extra private member... ... instantiated as an anon implementation of the interface. --- .../firestore/local/SQLitePersistence.java | 43 ++++++++----------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index 1a0d3a6d103..6b5f06eab5b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -76,6 +76,21 @@ public static String databaseName(String persistenceKey, DatabaseId databaseId) private final SQLiteQueryCache queryCache; private final SQLiteRemoteDocumentCache remoteDocumentCache; private final SQLiteLruReferenceDelegate referenceDelegate; + private final SQLiteTransactionListener transactionListener = + new SQLiteTransactionListener() { + @Override + public void onBegin() { + referenceDelegate.onTransactionStarted(); + } + + @Override + public void onCommit() { + referenceDelegate.onTransactionCommitted(); + } + + @Override + public void onRollback() {} + }; public SQLitePersistence( Context context, String persistenceKey, DatabaseId databaseId, LocalSerializer serializer) { @@ -142,33 +157,10 @@ RemoteDocumentCache getRemoteDocumentCache() { return remoteDocumentCache; } - private static class ReferenceDelegateToSQLiteTransactionListenerAdapter - implements SQLiteTransactionListener { - private ReferenceDelegate instance; - - ReferenceDelegateToSQLiteTransactionListenerAdapter(ReferenceDelegate instance) { - this.instance = instance; - } - - @Override - public void onBegin() { - instance.onTransactionStarted(); - } - - @Override - public void onCommit() { - instance.onTransactionCommitted(); - } - - @Override - public void onRollback() {} - } - @Override void runTransaction(String action, Runnable operation) { Logger.debug(TAG, "Starting transaction: %s", action); - db.beginTransactionWithListener( - new ReferenceDelegateToSQLiteTransactionListenerAdapter(referenceDelegate)); + db.beginTransactionWithListener(transactionListener); try { operation.run(); @@ -183,8 +175,7 @@ void runTransaction(String action, Runnable operation) { T runTransaction(String action, Supplier operation) { Logger.debug(TAG, "Starting transaction: %s", action); T value = null; - db.beginTransactionWithListener( - new ReferenceDelegateToSQLiteTransactionListenerAdapter(referenceDelegate)); + db.beginTransactionWithListener(transactionListener); try { value = operation.get();