From 74603b80ed998414bda3f294f8e6fa7d46370918 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Tue, 6 Aug 2019 10:53:41 -0400 Subject: [PATCH 1/7] awaitPendingWrites initial revision --- .../firebase/firestore/FirestoreTest.java | 21 ++++++++++ .../firebase/firestore/FirebaseFirestore.java | 12 ++++++ .../firestore/core/FirestoreClient.java | 18 +++++++++ .../firebase/firestore/core/SyncEngine.java | 40 +++++++++++++++++++ .../firebase/firestore/local/LocalStore.java | 8 ++++ .../firestore/local/MemoryMutationQueue.java | 5 +++ .../firestore/local/MutationQueue.java | 6 +++ .../firestore/local/SQLiteMutationQueue.java | 10 +++++ .../firestore/remote/RemoteStore.java | 2 +- .../firestore/local/LocalStoreTestCase.java | 17 ++++++++ 10 files changed, 138 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 71a50100837..d61ca6d600e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1096,4 +1096,25 @@ public void testShutdownCalledMultipleTimes() { expectError(() -> waitFor(reference.get()), expectedMessage); } + + @Test + public void testAwaitPendingWritesResolves() { + DocumentReference documentReference = testCollection("abc").document("123"); + FirebaseFirestore firestore = documentReference.getFirestore(); + Map data = map("foo", "bar"); + + waitFor(firestore.disableNetwork()); + Task awaitsPendingWrites1 = firestore.awaitPendingWrites(); + Task pendingWrite = documentReference.set(data); + Task awaitsPendingWrites2 = firestore.awaitPendingWrites(); + + assertTrue(!awaitsPendingWrites1.isComplete()); + assertTrue(!pendingWrite.isComplete()); + assertTrue(!awaitsPendingWrites2.isComplete()); + + waitFor(firestore.enableNetwork()); + waitFor(pendingWrite); + waitFor(awaitsPendingWrites1); + waitFor(awaitsPendingWrites2); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index efd1877425a..d57e0a4bcb4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -362,6 +362,18 @@ Task shutdown() { return shutdownInternal(); } + /** + * Wait for server acknowledgement for all pending writes existing at the time of calling this + * method. + * + *

Both acceptance and rejection count as server acknowledgement. + * + * @return A {@link Task} which resolves when all pending writes are acknowledged by the server. + */ + Task awaitPendingWrites() { + return client.awaitPendingWrites(); + } + @VisibleForTesting AsyncQueue getAsyncQueue() { return asyncQueue; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index fc85ac5cb21..04a351357ad 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -224,6 +224,24 @@ public Task transaction( () -> syncEngine.transaction(asyncQueue, updateFunction, retries)); } + /** + * Returns a task resolves when all the pending writes at the time when this method is called + * received server acknowledgement. An acknowledgement can be either acceptance or rejections. + */ + public Task awaitPendingWrites() { + this.verifyNotShutdown(); + if (!remoteStore.canUseNetwork()) { + Logger.warn( + LOG_TAG, + "Network is disabled, the Task created to wait for all writes getting" + + " acknowledged by server will not complete until network is enabled."); + } + + final TaskCompletionSource source = new TaskCompletionSource<>(); + asyncQueue.enqueueAndForget(() -> syncEngine.registerPendingWritesTask(source)); + return source.getTask(); + } + private void initialize(Context context, User user, boolean usePersistence, long cacheSizeBytes) { // Note: The initialization work must all be synchronous (we can't dispatch more work) since // external write/listen operations could get queued to run before that subsequent work diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index b11fc91c7fd..628e27a8f2b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -23,6 +23,7 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.common.base.Function; +import com.google.common.collect.Lists; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.FirebaseFirestoreException; @@ -133,6 +134,9 @@ interface SyncEngineCallback { /** Stores user completion blocks, indexed by user and batch ID. */ private final Map>> mutationUserCallbacks; + /** Stores user callbacks waiting for all pending writes to be acknowledged. */ + private final Map>> pendingWritesCallbacks; + /** Used for creating the target IDs for the listens used to resolve limbo documents. */ private final TargetIdGenerator targetIdGenerator; @@ -154,6 +158,8 @@ public SyncEngine(LocalStore localStore, RemoteStore remoteStore, User initialUs mutationUserCallbacks = new HashMap<>(); targetIdGenerator = TargetIdGenerator.forSyncEngine(); currentUser = initialUser; + + pendingWritesCallbacks = new HashMap<>(); } public void setCallback(SyncEngineCallback callback) { @@ -407,6 +413,8 @@ public void handleSuccessfulWrite(MutationBatchResult mutationBatchResult) { // they consistently happen before listen events. notifyUser(mutationBatchResult.getBatch().getBatchId(), /*status=*/ null); + resolveTasksAwaitingForPendingWritesIfAny(mutationBatchResult.getBatch().getBatchId()); + ImmutableSortedMap changes = localStore.acknowledgeBatch(mutationBatchResult); @@ -427,9 +435,41 @@ public void handleRejectedWrite(int batchId, Status status) { // they consistently happen before listen events. notifyUser(batchId, status); + resolveTasksAwaitingForPendingWritesIfAny(batchId); + emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); } + /** + * Takes a snapshot of current local mutation queue, and register a user task which will resolve + * when all those mutations are either accepted or rejected by the server. + */ + public void registerPendingWritesTask(TaskCompletionSource userTask) { + int largestPendingBatchId = localStore.getHighestUnacknowledgedBatchId(); + + if (largestPendingBatchId == 0) { + // Complete the task right away if there is no pending writes at the moment. + userTask.setResult(null); + } + + if (pendingWritesCallbacks.containsKey(largestPendingBatchId)) { + pendingWritesCallbacks.get(largestPendingBatchId).add(userTask); + } else { + pendingWritesCallbacks.put(largestPendingBatchId, Lists.newArrayList(userTask)); + } + } + + /** Resolves tasks waiting for this batch id to get acknowledged by server, if there is any. */ + private void resolveTasksAwaitingForPendingWritesIfAny(int batchId) { + if (pendingWritesCallbacks.containsKey(batchId)) { + for (TaskCompletionSource task : pendingWritesCallbacks.get(batchId)) { + task.setResult(null); + } + + pendingWritesCallbacks.remove(batchId); + } + } + /** Resolves the task corresponding to this write result. */ private void notifyUser(int batchId, @Nullable Status status) { Map> userTasks = mutationUserCallbacks.get(currentUser); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 0e92e76c2c2..5b24608ae9f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -282,6 +282,14 @@ public ImmutableSortedMap rejectBatch(int batchId) { }); } + /** + * Returns the largest (latest) batch id in mutation queue that is pending server response. + * Returns 0 if the queue is empty. + */ + public int getHighestUnacknowledgedBatchId() { + return mutationQueue.getLargestUnacknowledgedBatchId(); + } + /** Returns the last recorded stream token for the current user. */ public ByteString getLastStreamToken() { return mutationQueue.getLastStreamToken(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index 40330618760..d7ea5e680fd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -187,6 +187,11 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { return queue.size() > index ? queue.get(index) : null; } + @Override + public int getLargestUnacknowledgedBatchId() { + return queue.size() == 0 ? 0 : nextBatchId - 1; + } + @Override public List getAllMutationBatches() { return Collections.unmodifiableList(queue); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java index 0279914b96c..99092c5dbc8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java @@ -73,6 +73,12 @@ MutationBatch addMutationBatch( @Nullable MutationBatch getNextMutationBatchAfterBatchId(int batchId); + /** + * @return The largest (latest) batch id in mutation queue for the current user that is pending + * server response, 0 if the queue is empty. + */ + int getLargestUnacknowledgedBatchId(); + /** Returns all mutation batches in the mutation queue. */ // TODO: PERF: Current consumer only needs mutated keys; if we can provide that // cheaply, we should replace this. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index c6f208d4c59..142ee0c54d5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -249,6 +249,16 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { .firstValue(row -> decodeInlineMutationBatch(row.getInt(0), row.getBlob(1))); } + @Override + public int getLargestUnacknowledgedBatchId() { + if (isEmpty()) { + return 0; + } + return db.query("SELECT MAX(batch_id) FROM mutations " + "WHERE uid = ?") + .binding(uid) + .firstValue(row -> row.getInt(0)); + } + @Override public List getAllMutationBatches() { List result = new ArrayList<>(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java index a964a00d72b..476d88db342 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java @@ -478,7 +478,7 @@ private void handleWatchStreamClose(Status status) { } } - private boolean canUseNetwork() { + public boolean canUseNetwork() { // PORTING NOTE: This method exists mostly because web also has to take into account primary // vs. secondary state. return networkEnabled; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 93e45b30848..2db9a18c0db 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -1143,4 +1143,21 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS)); } + + @Test + public void testGetHighestUnacknowledgedBatchIdReturnsExpectedResult() { + assertEquals(0, localStore.getHighestUnacknowledgedBatchId()); + + writeMutation(setMutation("foo/bar", map("abc", 123))); + assertEquals(1, localStore.getHighestUnacknowledgedBatchId()); + + writeMutation(patchMutation("foo/bar", map("abc", 321))); + assertEquals(2, localStore.getHighestUnacknowledgedBatchId()); + + acknowledgeMutation(1); + assertEquals(2, localStore.getHighestUnacknowledgedBatchId()); + + rejectMutation(); + assertEquals(0, localStore.getHighestUnacknowledgedBatchId()); + } } From 9d8841eeb5f05dfd8d84db1ec9631fb3b6291523 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Tue, 6 Aug 2019 14:47:51 -0400 Subject: [PATCH 2/7] Add test to check task failure upon user change. --- .../firebase/firestore/FirestoreTest.java | 36 ++++++++++++++++--- .../testutil/IntegrationTestUtil.java | 36 ++++++++++++++++++- .../firebase/firestore/FirebaseFirestore.java | 4 +-- .../firestore/core/FirestoreClient.java | 2 +- .../firebase/firestore/core/SyncEngine.java | 16 +++++++++ 5 files changed, 85 insertions(+), 9 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index d61ca6d600e..3e6a1293a7a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.AccessHelper.getAsyncQueue; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.newTestSettings; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.provider; +import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testChangeUserTo; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollection; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testCollectionWithDocs; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.testDocument; @@ -44,6 +45,7 @@ import com.google.firebase.Timestamp; import com.google.firebase.firestore.FirebaseFirestoreException.Code; import com.google.firebase.firestore.Query.Direction; +import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.testutil.EventAccumulator; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import com.google.firebase.firestore.util.AsyncQueue.TimerId; @@ -1098,23 +1100,47 @@ public void testShutdownCalledMultipleTimes() { } @Test - public void testAwaitPendingWritesResolves() { + public void testWaitForPendingWritesResolves() { DocumentReference documentReference = testCollection("abc").document("123"); FirebaseFirestore firestore = documentReference.getFirestore(); Map data = map("foo", "bar"); waitFor(firestore.disableNetwork()); - Task awaitsPendingWrites1 = firestore.awaitPendingWrites(); + Task awaitsPendingWrites1 = firestore.waitForPendingWrites(); Task pendingWrite = documentReference.set(data); - Task awaitsPendingWrites2 = firestore.awaitPendingWrites(); + Task awaitsPendingWrites2 = firestore.waitForPendingWrites(); - assertTrue(!awaitsPendingWrites1.isComplete()); + // `awaitsPendingWrites1` is complete immediately because there is no pending writes at + // the time it is created. + waitFor(awaitsPendingWrites1); + assertTrue(awaitsPendingWrites1.isComplete() && awaitsPendingWrites1.isSuccessful()); assertTrue(!pendingWrite.isComplete()); assertTrue(!awaitsPendingWrites2.isComplete()); waitFor(firestore.enableNetwork()); waitFor(pendingWrite); - waitFor(awaitsPendingWrites1); waitFor(awaitsPendingWrites2); + assertTrue(awaitsPendingWrites2.isComplete() && awaitsPendingWrites2.isSuccessful()); + } + + @Test + public void testWaitForPendingWritesFailsWhenUserChanges() { + DocumentReference documentReference = testCollection("abc").document("123"); + FirebaseFirestore firestore = documentReference.getFirestore(); + Map data = map("foo", "bar"); + + // Prevent pending writes receiving acknowledgement. + waitFor(firestore.disableNetwork()); + Task pendingWrite = documentReference.set(data); + Task awaitsPendingWrites = firestore.waitForPendingWrites(); + assertTrue(!pendingWrite.isComplete()); + assertTrue(!awaitsPendingWrites.isComplete()); + + testChangeUserTo(new User("new user")); + + assertTrue(!pendingWrite.isComplete()); + assertEquals( + "'waitForPendingWrites' task is cancelled due to User change.", + waitForException(awaitsPendingWrites).getMessage()); } } diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 0971434e999..678b993bacd 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -36,11 +36,13 @@ import com.google.firebase.firestore.MetadataChanges; import com.google.firebase.firestore.QuerySnapshot; import com.google.firebase.firestore.auth.EmptyCredentialsProvider; +import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.local.Persistence; import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.testutil.provider.FirestoreProvider; import com.google.firebase.firestore.util.AsyncQueue; +import com.google.firebase.firestore.util.Listener; import com.google.firebase.firestore.util.Logger; import com.google.firebase.firestore.util.Logger.Level; import java.util.ArrayList; @@ -53,6 +55,34 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +class MockCredentialsProvider extends EmptyCredentialsProvider { + + private static MockCredentialsProvider instance; + + public static MockCredentialsProvider instance() { + if (MockCredentialsProvider.instance == null) { + MockCredentialsProvider.instance = new MockCredentialsProvider(); + } + return MockCredentialsProvider.instance; + } + + private MockCredentialsProvider() { + super(); + } + + @Override + public void setChangeListener(Listener changeListener) { + super.setChangeListener(changeListener); + this.listener = changeListener; + } + + public void changeUserTo(User user) { + listener.onValue(user); + } + + private Listener listener; +} + /** A set of helper methods for tests */ public class IntegrationTestUtil { @@ -239,7 +269,7 @@ public static FirebaseFirestore testFirestore( context, databaseId, persistenceKey, - new EmptyCredentialsProvider(), + MockCredentialsProvider.instance(), asyncQueue, /*firebaseApp=*/ null, /*instanceRegistry=*/ (dbId) -> {}); @@ -409,4 +439,8 @@ public static Map toDataMap(QuerySnapshot qrySnap) { public static boolean isRunningAgainstEmulator() { return CONNECT_TO_EMULATOR; } + + public static void testChangeUserTo(User user) { + MockCredentialsProvider.instance().changeUserTo(user); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index d57e0a4bcb4..9f81b9e449c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -370,8 +370,8 @@ Task shutdown() { * * @return A {@link Task} which resolves when all pending writes are acknowledged by the server. */ - Task awaitPendingWrites() { - return client.awaitPendingWrites(); + Task waitForPendingWrites() { + return client.waitForPendingWrites(); } @VisibleForTesting diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index 04a351357ad..cc00594bae6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -228,7 +228,7 @@ public Task transaction( * Returns a task resolves when all the pending writes at the time when this method is called * received server acknowledgement. An acknowledgement can be either acceptance or rejections. */ - public Task awaitPendingWrites() { + public Task waitForPendingWrites() { this.verifyNotShutdown(); if (!remoteStore.canUseNetwork()) { Logger.warn( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index 628e27a8f2b..587c5d0a962 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -450,6 +450,7 @@ public void registerPendingWritesTask(TaskCompletionSource userTask) { if (largestPendingBatchId == 0) { // Complete the task right away if there is no pending writes at the moment. userTask.setResult(null); + return; } if (pendingWritesCallbacks.containsKey(largestPendingBatchId)) { @@ -470,6 +471,20 @@ private void resolveTasksAwaitingForPendingWritesIfAny(int batchId) { } } + private void failOutstandingPendingWritesAwaitingTasks() { + for (Map.Entry>> entry : + pendingWritesCallbacks.entrySet()) { + for (TaskCompletionSource task : entry.getValue()) { + task.setException( + new FirebaseFirestoreException( + "'waitForPendingWrites' task is cancelled due to User change.", + FirebaseFirestoreException.Code.CANCELLED)); + } + } + + pendingWritesCallbacks.clear(); + } + /** Resolves the task corresponding to this write result. */ private void notifyUser(int batchId, @Nullable Status status) { Map> userTasks = mutationUserCallbacks.get(currentUser); @@ -605,6 +620,7 @@ public void handleCredentialChange(User user) { // Notify local store and emit any resulting events from swapping out the mutation queue. ImmutableSortedMap changes = localStore.handleUserChange(user); emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); + failOutstandingPendingWritesAwaitingTasks(); } // Notify remote store so it can restart its streams. From a683e2fea00bc26c1207f6163f82959b6462ea7a Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Tue, 6 Aug 2019 21:37:43 -0400 Subject: [PATCH 3/7] address comments #1 --- .../google/firebase/firestore/FirestoreTest.java | 3 +-- .../firebase/firestore/FirebaseFirestore.java | 8 +++----- .../firebase/firestore/core/FirestoreClient.java | 6 ------ .../firebase/firestore/core/SyncEngine.java | 16 +++++++++++----- .../firebase/firestore/local/LocalStore.java | 2 +- .../firestore/local/MemoryMutationQueue.java | 4 ++-- .../firebase/firestore/local/MutationQueue.java | 2 +- .../firestore/local/SQLiteMutationQueue.java | 7 ++----- .../firestore/local/LocalStoreTestCase.java | 2 +- 9 files changed, 22 insertions(+), 28 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 3e6a1293a7a..0e034445f5e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1117,8 +1117,7 @@ public void testWaitForPendingWritesResolves() { assertTrue(!pendingWrite.isComplete()); assertTrue(!awaitsPendingWrites2.isComplete()); - waitFor(firestore.enableNetwork()); - waitFor(pendingWrite); + firestore.enableNetwork(); waitFor(awaitsPendingWrites2); assertTrue(awaitsPendingWrites2.isComplete() && awaitsPendingWrites2.isSuccessful()); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 9f81b9e449c..a057a59a7ea 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -363,12 +363,10 @@ Task shutdown() { } /** - * Wait for server acknowledgement for all pending writes existing at the time of calling this - * method. + * Wait until all pending writes existed at the time of calling are sent to the backend. * - *

Both acceptance and rejection count as server acknowledgement. - * - * @return A {@link Task} which resolves when all pending writes are acknowledged by the server. + * @return A {@link Task} which resolves when all pending writes are sent to the backend. If there + * is a Firebase user change, the return {@link Task} will resolve to an exception. */ Task waitForPendingWrites() { return client.waitForPendingWrites(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index cc00594bae6..fb8b23c7948 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -230,12 +230,6 @@ public Task transaction( */ public Task waitForPendingWrites() { this.verifyNotShutdown(); - if (!remoteStore.canUseNetwork()) { - Logger.warn( - LOG_TAG, - "Network is disabled, the Task created to wait for all writes getting" - + " acknowledged by server will not complete until network is enabled."); - } final TaskCompletionSource source = new TaskCompletionSource<>(); asyncQueue.enqueueAndForget(() -> syncEngine.registerPendingWritesTask(source)); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index 587c5d0a962..437ad125175 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -413,7 +413,7 @@ public void handleSuccessfulWrite(MutationBatchResult mutationBatchResult) { // they consistently happen before listen events. notifyUser(mutationBatchResult.getBatch().getBatchId(), /*status=*/ null); - resolveTasksAwaitingForPendingWritesIfAny(mutationBatchResult.getBatch().getBatchId()); + resolvePendingWriteTasks(mutationBatchResult.getBatch().getBatchId()); ImmutableSortedMap changes = localStore.acknowledgeBatch(mutationBatchResult); @@ -435,16 +435,22 @@ public void handleRejectedWrite(int batchId, Status status) { // they consistently happen before listen events. notifyUser(batchId, status); - resolveTasksAwaitingForPendingWritesIfAny(batchId); + resolvePendingWriteTasks(batchId); emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); } /** - * Takes a snapshot of current local mutation queue, and register a user task which will resolve - * when all those mutations are either accepted or rejected by the server. + * Takes a snapshot of current mutation queue, and register a user task which will resolve when + * all those mutations are either accepted or rejected by the server. */ public void registerPendingWritesTask(TaskCompletionSource userTask) { + if (!remoteStore.canUseNetwork()) { + Logger.debug( + TAG, + "The network is disabled. The task returned by 'awaitPendingWrites()' will not complete until the network is enabled."); + } + int largestPendingBatchId = localStore.getHighestUnacknowledgedBatchId(); if (largestPendingBatchId == 0) { @@ -461,7 +467,7 @@ public void registerPendingWritesTask(TaskCompletionSource userTask) { } /** Resolves tasks waiting for this batch id to get acknowledged by server, if there is any. */ - private void resolveTasksAwaitingForPendingWritesIfAny(int batchId) { + private void resolvePendingWriteTasks(int batchId) { if (pendingWritesCallbacks.containsKey(batchId)) { for (TaskCompletionSource task : pendingWritesCallbacks.get(batchId)) { task.setResult(null); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 5b24608ae9f..e5625e3109f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -287,7 +287,7 @@ public ImmutableSortedMap rejectBatch(int batchId) { * Returns 0 if the queue is empty. */ public int getHighestUnacknowledgedBatchId() { - return mutationQueue.getLargestUnacknowledgedBatchId(); + return mutationQueue.getHighestUnacknowledgedBatchId(); } /** Returns the last recorded stream token for the current user. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index d7ea5e680fd..c31337c2132 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -188,8 +188,8 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { } @Override - public int getLargestUnacknowledgedBatchId() { - return queue.size() == 0 ? 0 : nextBatchId - 1; + public int getHighestUnacknowledgedBatchId() { + return queue.isEmpty() ? 0 : nextBatchId - 1; } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java index 99092c5dbc8..71f0646080f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java @@ -77,7 +77,7 @@ MutationBatch addMutationBatch( * @return The largest (latest) batch id in mutation queue for the current user that is pending * server response, 0 if the queue is empty. */ - int getLargestUnacknowledgedBatchId(); + int getHighestUnacknowledgedBatchId(); /** Returns all mutation batches in the mutation queue. */ // TODO: PERF: Current consumer only needs mutated keys; if we can provide that diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index 142ee0c54d5..b3e5b157030 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -250,11 +250,8 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { } @Override - public int getLargestUnacknowledgedBatchId() { - if (isEmpty()) { - return 0; - } - return db.query("SELECT MAX(batch_id) FROM mutations " + "WHERE uid = ?") + public int getHighestUnacknowledgedBatchId() { + return db.query("SELECT IFNULL(MAX(batch_id), 0) FROM mutations WHERE uid = ?") .binding(uid) .firstValue(row -> row.getInt(0)); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 2db9a18c0db..53bd3f144a4 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -1145,7 +1145,7 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { } @Test - public void testGetHighestUnacknowledgedBatchIdReturnsExpectedResult() { + public void testGetHighestUnacknowledgedBatchId() { assertEquals(0, localStore.getHighestUnacknowledgedBatchId()); writeMutation(setMutation("foo/bar", map("abc", 123))); From 86e3abf7b899b74a355901e96587de14512ced60 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 7 Aug 2019 15:07:41 -0400 Subject: [PATCH 4/7] addressing comments #2 --- .../com/google/firebase/firestore/FirestoreTest.java | 2 +- .../com/google/firebase/firestore/core/SyncEngine.java | 9 ++++++--- .../com/google/firebase/firestore/local/LocalStore.java | 2 +- .../firebase/firestore/local/MemoryMutationQueue.java | 2 +- .../google/firebase/firestore/local/MutationQueue.java | 2 +- .../firebase/firestore/local/SQLiteMutationQueue.java | 4 ++-- .../firebase/firestore/local/LocalStoreTestCase.java | 4 ++-- 7 files changed, 14 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 0e034445f5e..c299625ad42 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1110,7 +1110,7 @@ public void testWaitForPendingWritesResolves() { Task pendingWrite = documentReference.set(data); Task awaitsPendingWrites2 = firestore.waitForPendingWrites(); - // `awaitsPendingWrites1` is complete immediately because there is no pending writes at + // `awaitsPendingWrites1` completes immediately because there are no pending writes at // the time it is created. waitFor(awaitsPendingWrites1); assertTrue(awaitsPendingWrites1.isComplete() && awaitsPendingWrites1.isSuccessful()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index 437ad125175..356066eb22c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -40,6 +40,7 @@ import com.google.firebase.firestore.model.NoDocument; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.mutation.Mutation; +import com.google.firebase.firestore.model.mutation.MutationBatch; import com.google.firebase.firestore.model.mutation.MutationBatchResult; import com.google.firebase.firestore.remote.Datastore; import com.google.firebase.firestore.remote.RemoteEvent; @@ -448,12 +449,13 @@ public void registerPendingWritesTask(TaskCompletionSource userTask) { if (!remoteStore.canUseNetwork()) { Logger.debug( TAG, - "The network is disabled. The task returned by 'awaitPendingWrites()' will not complete until the network is enabled."); + "The network is disabled. The task returned by 'awaitPendingWrites()' will not" + + " complete until the network is enabled."); } int largestPendingBatchId = localStore.getHighestUnacknowledgedBatchId(); - if (largestPendingBatchId == 0) { + if (largestPendingBatchId == MutationBatch.UNKNOWN) { // Complete the task right away if there is no pending writes at the moment. userTask.setResult(null); return; @@ -623,10 +625,11 @@ public void handleCredentialChange(User user) { currentUser = user; if (userChanged) { + // Fails tasks waiting for pending writes requested by previous user. + failOutstandingPendingWritesAwaitingTasks(); // Notify local store and emit any resulting events from swapping out the mutation queue. ImmutableSortedMap changes = localStore.handleUserChange(user); emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); - failOutstandingPendingWritesAwaitingTasks(); } // Notify remote store so it can restart its streams. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index e5625e3109f..8d2d2aafecb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -284,7 +284,7 @@ public ImmutableSortedMap rejectBatch(int batchId) { /** * Returns the largest (latest) batch id in mutation queue that is pending server response. - * Returns 0 if the queue is empty. + * Returns {@link MutationBatch#UNKNOWN} if the queue is empty. */ public int getHighestUnacknowledgedBatchId() { return mutationQueue.getHighestUnacknowledgedBatchId(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index c31337c2132..f113580f422 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -189,7 +189,7 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { @Override public int getHighestUnacknowledgedBatchId() { - return queue.isEmpty() ? 0 : nextBatchId - 1; + return queue.isEmpty() ? MutationBatch.UNKNOWN : nextBatchId - 1; } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java index 71f0646080f..c400845a4b8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java @@ -75,7 +75,7 @@ MutationBatch addMutationBatch( /** * @return The largest (latest) batch id in mutation queue for the current user that is pending - * server response, 0 if the queue is empty. + * server response, {@link MutationBatch#UNKNOWN} if the queue is empty. */ int getHighestUnacknowledgedBatchId(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index b3e5b157030..3f7fe49c79f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -251,8 +251,8 @@ public MutationBatch getNextMutationBatchAfterBatchId(int batchId) { @Override public int getHighestUnacknowledgedBatchId() { - return db.query("SELECT IFNULL(MAX(batch_id), 0) FROM mutations WHERE uid = ?") - .binding(uid) + return db.query("SELECT IFNULL(MAX(batch_id), ?) FROM mutations WHERE uid = ?") + .binding(MutationBatch.UNKNOWN, uid) .firstValue(row -> row.getInt(0)); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 53bd3f144a4..b908a14db1c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -1146,7 +1146,7 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { @Test public void testGetHighestUnacknowledgedBatchId() { - assertEquals(0, localStore.getHighestUnacknowledgedBatchId()); + assertEquals(-1, localStore.getHighestUnacknowledgedBatchId()); writeMutation(setMutation("foo/bar", map("abc", 123))); assertEquals(1, localStore.getHighestUnacknowledgedBatchId()); @@ -1158,6 +1158,6 @@ public void testGetHighestUnacknowledgedBatchId() { assertEquals(2, localStore.getHighestUnacknowledgedBatchId()); rejectMutation(); - assertEquals(0, localStore.getHighestUnacknowledgedBatchId()); + assertEquals(-1, localStore.getHighestUnacknowledgedBatchId()); } } From 67fec18ccc1f74ff3d2f390bfc955c8b8bb01376 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 7 Aug 2019 16:22:46 -0400 Subject: [PATCH 5/7] addressing comments #3 --- .../com/google/firebase/firestore/FirebaseFirestore.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index a057a59a7ea..fe4fe77b7c9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -363,10 +363,12 @@ Task shutdown() { } /** - * Wait until all pending writes existed at the time of calling are sent to the backend. + * Waits until all pending writes that existed at the time of calling have been successfully + * written to the server. * - * @return A {@link Task} which resolves when all pending writes are sent to the backend. If there - * is a Firebase user change, the return {@link Task} will resolve to an exception. + * @return A {@code Task} which resolves when all pending writes are written to the backend. If + * there is a Firebase user change, the returned {@code Task} will resolve to an exception. If + * the client is offline, the returned {@code Task} will not resolve. */ Task waitForPendingWrites() { return client.waitForPendingWrites(); From ec2d7d26166ee974ab1421ccb470104b8e6e83ba Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 7 Aug 2019 21:57:50 -0400 Subject: [PATCH 6/7] addressing comments #4: better public comment. --- .../firebase/firestore/FirebaseFirestore.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index fe4fe77b7c9..47493ad5e82 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -363,12 +363,18 @@ Task shutdown() { } /** - * Waits until all pending writes that existed at the time of calling have been successfully - * written to the server. + * Waits until all currently pending writes for the active user have been acknowledged by the + * backend. * - * @return A {@code Task} which resolves when all pending writes are written to the backend. If - * there is a Firebase user change, the returned {@code Task} will resolve to an exception. If - * the client is offline, the returned {@code Task} will not resolve. + *

The returned Task completes immediately if there are no outstanding writes. Otherwise, the + * Task waits for all previously issued writes (including those written in a previous app + * session), but it does not wait for writes that were added after the method is called. If you + * wish to wait for additional writes, you have to call `waitForPendingWrites()` again. + * + *

Any outstanding `waitForPendingWrites()` Tasks are cancelled during user changes. + * + * @return A {@code Task} which resolves when all currently pending writes have been acknowledged + * by the backend. */ Task waitForPendingWrites() { return client.waitForPendingWrites(); From a4e43552a58da65f50d9dbdf87999e4afcfdd517 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 8 Aug 2019 10:35:34 -0400 Subject: [PATCH 7/7] fixing more nits --- .../google/firebase/firestore/FirestoreTest.java | 14 ++++++++++++++ .../firestore/testutil/IntegrationTestUtil.java | 7 ++----- .../firebase/firestore/FirebaseFirestore.java | 4 ++-- .../google/firebase/firestore/core/SyncEngine.java | 6 +++--- .../firestore/local/LocalStoreTestCase.java | 4 ++-- 5 files changed, 23 insertions(+), 12 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index c299625ad42..3b2c9d5c3e7 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -1142,4 +1142,18 @@ public void testWaitForPendingWritesFailsWhenUserChanges() { "'waitForPendingWrites' task is cancelled due to User change.", waitForException(awaitsPendingWrites).getMessage()); } + + @Test + public void testPendingWriteTaskResolveWhenOfflineIfThereIsNoPending() { + DocumentReference documentReference = testCollection("abc").document("123"); + FirebaseFirestore firestore = documentReference.getFirestore(); + Map data = map("foo", "bar"); + + // Prevent pending writes receiving acknowledgement. + waitFor(firestore.disableNetwork()); + Task awaitsPendingWrites = firestore.waitForPendingWrites(); + waitFor(awaitsPendingWrites); + + assertTrue(awaitsPendingWrites.isComplete() && awaitsPendingWrites.isSuccessful()); + } } diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 678b993bacd..f3c54c8e12b 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -58,6 +58,7 @@ class MockCredentialsProvider extends EmptyCredentialsProvider { private static MockCredentialsProvider instance; + private Listener listener; public static MockCredentialsProvider instance() { if (MockCredentialsProvider.instance == null) { @@ -66,9 +67,7 @@ public static MockCredentialsProvider instance() { return MockCredentialsProvider.instance; } - private MockCredentialsProvider() { - super(); - } + private MockCredentialsProvider() {} @Override public void setChangeListener(Listener changeListener) { @@ -79,8 +78,6 @@ public void setChangeListener(Listener changeListener) { public void changeUserTo(User user) { listener.onValue(user); } - - private Listener listener; } /** A set of helper methods for tests */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 47493ad5e82..d1b3099b20d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -369,9 +369,9 @@ Task shutdown() { *

The returned Task completes immediately if there are no outstanding writes. Otherwise, the * Task waits for all previously issued writes (including those written in a previous app * session), but it does not wait for writes that were added after the method is called. If you - * wish to wait for additional writes, you have to call `waitForPendingWrites()` again. + * wish to wait for additional writes, you have to call {@code waitForPendingWrites()} again. * - *

Any outstanding `waitForPendingWrites()` Tasks are cancelled during user changes. + *

Any outstanding {@code waitForPendingWrites()} Tasks are cancelled during user changes. * * @return A {@code Task} which resolves when all currently pending writes have been acknowledged * by the backend. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index 356066eb22c..e03694637e4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -449,8 +449,8 @@ public void registerPendingWritesTask(TaskCompletionSource userTask) { if (!remoteStore.canUseNetwork()) { Logger.debug( TAG, - "The network is disabled. The task returned by 'awaitPendingWrites()' will not" - + " complete until the network is enabled."); + "The network is disabled. The task returned by 'awaitPendingWrites()' will not " + + "complete until the network is enabled."); } int largestPendingBatchId = localStore.getHighestUnacknowledgedBatchId(); @@ -468,7 +468,7 @@ public void registerPendingWritesTask(TaskCompletionSource userTask) { } } - /** Resolves tasks waiting for this batch id to get acknowledged by server, if there is any. */ + /** Resolves tasks waiting for this batch id to get acknowledged by server, if there are any. */ private void resolvePendingWriteTasks(int batchId) { if (pendingWritesCallbacks.containsKey(batchId)) { for (TaskCompletionSource task : pendingWritesCallbacks.get(batchId)) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index b908a14db1c..8a6d93fce30 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -1146,7 +1146,7 @@ public void testHandlesPatchMutationWithTransformThenRemoteEvent() { @Test public void testGetHighestUnacknowledgedBatchId() { - assertEquals(-1, localStore.getHighestUnacknowledgedBatchId()); + assertEquals(MutationBatch.UNKNOWN, localStore.getHighestUnacknowledgedBatchId()); writeMutation(setMutation("foo/bar", map("abc", 123))); assertEquals(1, localStore.getHighestUnacknowledgedBatchId()); @@ -1158,6 +1158,6 @@ public void testGetHighestUnacknowledgedBatchId() { assertEquals(2, localStore.getHighestUnacknowledgedBatchId()); rejectMutation(); - assertEquals(-1, localStore.getHighestUnacknowledgedBatchId()); + assertEquals(MutationBatch.UNKNOWN, localStore.getHighestUnacknowledgedBatchId()); } }