From 20da793012ac83940d3ef53c2bdb6db6d8cc00d8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 Feb 2021 00:45:01 -0500 Subject: [PATCH 1/7] Remove enqueued limbo resolutions when a query stops and avoid starting one if already enqueued. --- .../firebase/firestore/core/SyncEngine.java | 24 +- .../test/resources/json/limbo_spec_test.json | 2231 +++++++++++++++++ 2 files changed, 2245 insertions(+), 10 deletions(-) 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 e6878d9b746..545ec110fe1 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 @@ -21,6 +21,8 @@ import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.FirebaseFirestoreException; @@ -59,9 +61,9 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; -import java.util.Queue; import java.util.Set; /** @@ -130,7 +132,7 @@ interface SyncEngineCallback { * The keys of documents that are in limbo for which we haven't yet started a limbo resolution * query. */ - private final Queue enqueuedLimboResolutions; + private final LinkedHashSet enqueuedLimboResolutions; /** Keeps track of the target ID for each document that is in limbo with an active target. */ private final Map activeLimboTargetsByKey; @@ -169,7 +171,7 @@ public SyncEngine( queryViewsByQuery = new HashMap<>(); queriesByTarget = new HashMap<>(); - enqueuedLimboResolutions = new ArrayDeque<>(); + enqueuedLimboResolutions = new LinkedHashSet<>(); activeLimboTargetsByKey = new HashMap<>(); activeLimboResolutionsByTarget = new HashMap<>(); limboDocumentRefs = new ReferenceSet(); @@ -603,6 +605,7 @@ private void removeAndCleanupTarget(int targetId, Status status) { } private void removeLimboTarget(DocumentKey key) { + enqueuedLimboResolutions.remove(key); // It's possible that the target already got removed because the query failed. In that case, // the key won't exist in `limboTargetsByKey`. Only do the cleanup if we still have the target. Integer targetId = activeLimboTargetsByKey.get(key); @@ -676,7 +679,7 @@ private void updateTrackedLimboDocuments(List limboChanges, private void trackLimboChange(LimboDocumentChange change) { DocumentKey key = change.getKey(); - if (!activeLimboTargetsByKey.containsKey(key)) { + if (!activeLimboTargetsByKey.containsKey(key) && !enqueuedLimboResolutions.contains(key)) { Logger.debug(TAG, "New document in limbo: %s", key); enqueuedLimboResolutions.add(key); pumpEnqueuedLimboResolutions(); @@ -694,7 +697,8 @@ private void trackLimboChange(LimboDocumentChange change) { private void pumpEnqueuedLimboResolutions() { while (!enqueuedLimboResolutions.isEmpty() && activeLimboTargetsByKey.size() < maxConcurrentLimboResolutions) { - DocumentKey key = enqueuedLimboResolutions.remove(); + DocumentKey key = enqueuedLimboResolutions.iterator().next(); + enqueuedLimboResolutions.remove(key); int limboTargetId = targetIdGenerator.nextId(); activeLimboResolutionsByTarget.put(limboTargetId, new LimboResolution(key)); activeLimboTargetsByKey.put(key, limboTargetId); @@ -708,15 +712,15 @@ private void pumpEnqueuedLimboResolutions() { } @VisibleForTesting - public Map getActiveLimboDocumentResolutions() { + public ImmutableMap getActiveLimboDocumentResolutions() { // Make a defensive copy as the Map continues to be modified. - return new HashMap<>(activeLimboTargetsByKey); + return ImmutableMap.copyOf(activeLimboTargetsByKey); } @VisibleForTesting - public Queue getEnqueuedLimboDocumentResolutions() { - // Make a defensive copy as the Queue continues to be modified. - return new ArrayDeque<>(enqueuedLimboResolutions); + public ImmutableSet getEnqueuedLimboDocumentResolutions() { + // Make a defensive copy as the LinkedHashMap continues to be modified. + return ImmutableSet.copyOf(enqueuedLimboResolutions); } public void handleCredentialChange(User user) { diff --git a/firebase-firestore/src/test/resources/json/limbo_spec_test.json b/firebase-firestore/src/test/resources/json/limbo_spec_test.json index aded75fb9e5..57f57e909e5 100644 --- a/firebase-firestore/src/test/resources/json/limbo_spec_test.json +++ b/firebase-firestore/src/test/resources/json/limbo_spec_test.json @@ -1,4 +1,2235 @@ { + "A limbo resolution for a document should be removed from the queue when the last query listen stops": { + "describeName": "Limbo Documents:", + "itName": "A limbo resolution for a document should be removed from the queue when the last query listen stops", + "tags": [ + ], + "config": { + "maxConcurrentLimboResolutions": 1, + "numClients": 1, + "useGarbageCollection": false + }, + "steps": [ + { + "userListen": { + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + }, + "targetId": 2 + }, + "expectedState": { + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchEntity": { + "docs": [ + { + "key": "collection1/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "targets": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1000 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection1/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + } + } + ] + }, + { + "userUnlisten": [ + 2, + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "expectedState": { + "activeTargets": { + } + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + }, + "targetId": 4 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection1/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + } + ], + "expectedState": { + "activeTargets": { + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 4 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 4 + ] + } + }, + { + "watchCurrent": [ + [ + 4 + ], + "resume-token-1001" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1001 + }, + "expectedState": { + "activeLimboDocs": [ + "collection1/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "userListen": { + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + }, + "targetId": 6 + }, + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "6": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 6 + ] + }, + { + "watchEntity": { + "docs": [ + { + "key": "collection2/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "targets": [ + 6 + ] + } + }, + { + "watchCurrent": [ + [ + 6 + ], + "resume-token-1002" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1002 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection2/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + } + } + ] + }, + { + "userUnlisten": [ + 6, + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + }, + "targetId": 8 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection2/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 8 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 8 + ] + } + }, + { + "watchCurrent": [ + [ + 8 + ], + "resume-token-1003" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1003 + }, + "expectedState": { + "activeLimboDocs": [ + "collection1/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + "collection2/doc" + ] + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + }, + "targetId": 10 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection2/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "10": { + "queries": [ + { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 10 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 10 + ] + } + }, + { + "watchCurrent": [ + [ + 10 + ], + "resume-token-1004" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1004 + }, + "expectedState": { + "activeLimboDocs": [ + "collection1/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "10": { + "queries": [ + { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + "collection2/doc" + ] + } + }, + { + "userUnlisten": [ + 10, + { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "expectedState": { + "activeLimboDocs": [ + "collection1/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + "collection2/doc" + ] + } + }, + { + "userUnlisten": [ + 8, + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "expectedState": { + "activeLimboDocs": [ + "collection1/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + ] + } + } + ] + }, + "A limbo resolution for a document should not be enqueued if one is already enqueued": { + "describeName": "Limbo Documents:", + "itName": "A limbo resolution for a document should not be enqueued if one is already enqueued", + "tags": [ + ], + "config": { + "maxConcurrentLimboResolutions": 1, + "numClients": 1, + "useGarbageCollection": false + }, + "steps": [ + { + "userListen": { + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + }, + "targetId": 2 + }, + "expectedState": { + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchEntity": { + "docs": [ + { + "key": "collection1/doc1", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "targets": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1000 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection1/doc1", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + } + } + ] + }, + { + "userUnlisten": [ + 2, + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "expectedState": { + "activeTargets": { + } + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + }, + "targetId": 4 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection1/doc1", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + } + ], + "expectedState": { + "activeTargets": { + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 4 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 4 + ] + } + }, + { + "watchCurrent": [ + [ + 4 + ], + "resume-token-1001" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1001 + }, + "expectedState": { + "activeLimboDocs": [ + "collection1/doc1" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "userListen": { + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + }, + "targetId": 6 + }, + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "6": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 6 + ] + }, + { + "watchEntity": { + "docs": [ + { + "key": "collection2/doc2", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "targets": [ + 6 + ] + } + }, + { + "watchCurrent": [ + [ + 6 + ], + "resume-token-1002" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1002 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection2/doc2", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + } + } + ] + }, + { + "userUnlisten": [ + 6, + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + } + } + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + }, + "targetId": 8 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection2/doc2", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 8 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 8 + ] + } + }, + { + "watchCurrent": [ + [ + 8 + ], + "resume-token-1003" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1003 + }, + "expectedState": { + "activeLimboDocs": [ + "collection1/doc1" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + "collection2/doc2" + ] + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + }, + "targetId": 10 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection2/doc2", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "10": { + "queries": [ + { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 10 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 10 + ] + } + }, + { + "watchCurrent": [ + [ + 10 + ], + "resume-token-1004" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1004 + }, + "expectedState": { + "activeLimboDocs": [ + "collection1/doc1" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection1/doc1" + } + ], + "resumeToken": "" + }, + "10": { + "queries": [ + { + "filters": [ + [ + "key", + ">=", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection1" + } + ], + "resumeToken": "" + }, + "8": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 2 + ] + ], + "orderBys": [ + ], + "path": "collection2" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + "collection2/doc2" + ] + } + } + ] + }, + "A limbo resolution for a document should not be started if one is already active": { + "describeName": "Limbo Documents:", + "itName": "A limbo resolution for a document should not be started if one is already active", + "tags": [ + ], + "config": { + "numClients": 1, + "useGarbageCollection": false + }, + "steps": [ + { + "userListen": { + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + }, + "targetId": 2 + }, + "expectedState": { + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchEntity": { + "docs": [ + { + "key": "collection/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "targets": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1000 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + } + ] + }, + { + "userUnlisten": [ + 2, + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "expectedState": { + "activeTargets": { + } + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + }, + "targetId": 4 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + } + ], + "expectedState": { + "activeTargets": { + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 4 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 4 + ] + } + }, + { + "watchCurrent": [ + [ + 4 + ], + "resume-token-1001" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1001 + }, + "expectedState": { + "activeLimboDocs": [ + "collection/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + ] + } + }, + { + "userListen": { + "query": { + "filters": [ + [ + "key", + ">=", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + }, + "targetId": 6 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "key": "collection/doc", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "key": 1 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + [ + "key", + ">=", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + } + ], + "expectedState": { + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + }, + "6": { + "queries": [ + { + "filters": [ + [ + "key", + ">=", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 6 + ] + }, + { + "watchEntity": { + "docs": [ + ], + "targets": [ + 6 + ] + } + }, + { + "watchCurrent": [ + [ + 6 + ], + "resume-token-1002" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1002 + }, + "expectedState": { + "activeLimboDocs": [ + "collection/doc" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection/doc" + } + ], + "resumeToken": "" + }, + "4": { + "queries": [ + { + "filters": [ + [ + "key", + "==", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + }, + "6": { + "queries": [ + { + "filters": [ + [ + "key", + ">=", + 1 + ] + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + } + }, + "enqueuedLimboDocs": [ + ] + } + } + ] + }, "Document remove message will cause docs to go in limbo": { "describeName": "Limbo Documents:", "itName": "Document remove message will cause docs to go in limbo", From 4e1f815aacea34b66ef8bf4b152ebd41cdd97822 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 Feb 2021 00:51:40 -0500 Subject: [PATCH 2/7] Add change log entry --- firebase-firestore/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 41943d0f2e2..ff1f9ba9f07 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -4,6 +4,9 @@ Bundles contain pre-packaged data produced with the NodeJS Server SDK and can be used to populate Firestore's cache without reading documents from the backend. +- [fixed] Fix a Firestore bug where local cache inconsistencies were + unnecessarily being resolved, resulting in the incompletion of `Task` objects + returned from `get()` invocations. # (22.0.2) - [changed] A write to a document that contains FieldValue transforms is no From 9a438c5c92063f03f4ed725414c9a71c49844e6f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 Feb 2021 00:53:27 -0500 Subject: [PATCH 3/7] Add a link to the PR in the change log entry --- firebase-firestore/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index ff1f9ba9f07..a48740385f5 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -6,7 +6,7 @@ the backend. - [fixed] Fix a Firestore bug where local cache inconsistencies were unnecessarily being resolved, resulting in the incompletion of `Task` objects - returned from `get()` invocations. + returned from `get()` invocations (#2404). # (22.0.2) - [changed] A write to a document that contains FieldValue transforms is no From 2fc9656c1b1507b3c068ef1a832b77c41aac5dec Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 Feb 2021 02:04:27 -0500 Subject: [PATCH 4/7] Code format via ./gradlew :firebase-firestore:googleJavaFormat --- .../main/java/com/google/firebase/firestore/core/SyncEngine.java | 1 - 1 file changed, 1 deletion(-) 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 545ec110fe1..9079b7572cf 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 @@ -57,7 +57,6 @@ import com.google.firebase.firestore.util.Util; import io.grpc.Status; import java.io.IOException; -import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; From 414b86a21c2b3da58175ab5386aa5f1499cf77ca Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Feb 2021 10:06:10 -0500 Subject: [PATCH 5/7] SyncEngine.java: Clean up iterator usage in `pumpEnqueuedLimboResolutions()`. Co-authored-by: Sebastian Schmidt --- .../java/com/google/firebase/firestore/core/SyncEngine.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 9079b7572cf..646f367f139 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 @@ -696,8 +696,9 @@ private void trackLimboChange(LimboDocumentChange change) { private void pumpEnqueuedLimboResolutions() { while (!enqueuedLimboResolutions.isEmpty() && activeLimboTargetsByKey.size() < maxConcurrentLimboResolutions) { - DocumentKey key = enqueuedLimboResolutions.iterator().next(); - enqueuedLimboResolutions.remove(key); + Iterator it = enqueuedLimboResolutions.iterator(); + DocumentKey key = it.next(); + it.remove(); int limboTargetId = targetIdGenerator.nextId(); activeLimboResolutionsByTarget.put(limboTargetId, new LimboResolution(key)); activeLimboTargetsByKey.put(key, limboTargetId); From e32d40733d45c7a384bc25352511075f783ad34e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Feb 2021 10:11:06 -0500 Subject: [PATCH 6/7] Apply code review feedback. --- firebase-firestore/CHANGELOG.md | 6 +++--- .../google/firebase/firestore/core/SyncEngine.java | 12 +++++------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index a48740385f5..b5317a96988 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -4,9 +4,9 @@ Bundles contain pre-packaged data produced with the NodeJS Server SDK and can be used to populate Firestore's cache without reading documents from the backend. -- [fixed] Fix a Firestore bug where local cache inconsistencies were - unnecessarily being resolved, resulting in the incompletion of `Task` objects - returned from `get()` invocations (#2404). +- [fixed] Fixed a Firestore bug where local cache inconsistencies were + unnecessarily being resolved, causing the `Task` objects returned from `get()` + invocations to never complete (#2404). # (22.0.2) - [changed] A write to a document that contains FieldValue transforms is no 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 646f367f139..bee58732a7d 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 @@ -21,8 +21,6 @@ import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.FirebaseFirestoreException; @@ -712,15 +710,15 @@ private void pumpEnqueuedLimboResolutions() { } @VisibleForTesting - public ImmutableMap getActiveLimboDocumentResolutions() { + public HashMap getActiveLimboDocumentResolutions() { // Make a defensive copy as the Map continues to be modified. - return ImmutableMap.copyOf(activeLimboTargetsByKey); + return new HashMap(activeLimboTargetsByKey); } @VisibleForTesting - public ImmutableSet getEnqueuedLimboDocumentResolutions() { - // Make a defensive copy as the LinkedHashMap continues to be modified. - return ImmutableSet.copyOf(enqueuedLimboResolutions); + public LinkedHashSet getEnqueuedLimboDocumentResolutions() { + // Make a defensive copy as the LinkedHashSet continues to be modified. + return new LinkedHashSet(enqueuedLimboResolutions); } public void handleCredentialChange(User user) { From b428adc9fed6b806b5705b365a39b6cc4f9b73e6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 5 Feb 2021 11:19:45 -0500 Subject: [PATCH 7/7] SyncEngine.java: Add missing import of java.util.Iterator and change some return types to more general types (e.g. LinkedHashSet -> List). --- .../com/google/firebase/firestore/core/SyncEngine.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 bee58732a7d..ca374de64a7 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 @@ -58,6 +58,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -710,15 +711,15 @@ private void pumpEnqueuedLimboResolutions() { } @VisibleForTesting - public HashMap getActiveLimboDocumentResolutions() { + public Map getActiveLimboDocumentResolutions() { // Make a defensive copy as the Map continues to be modified. - return new HashMap(activeLimboTargetsByKey); + return new HashMap<>(activeLimboTargetsByKey); } @VisibleForTesting - public LinkedHashSet getEnqueuedLimboDocumentResolutions() { + public List getEnqueuedLimboDocumentResolutions() { // Make a defensive copy as the LinkedHashSet continues to be modified. - return new LinkedHashSet(enqueuedLimboResolutions); + return new ArrayList<>(enqueuedLimboResolutions); } public void handleCredentialChange(User user) {