Dequeue limbo resolutions when their respective queries are stopped (#2404)

Author: dconeybe

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] 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

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

@@ -55,13 +55,13 @@
 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;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Queue;
 import java.util.Set;
 
 /**
@@ -130,7 +130,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<DocumentKey> enqueuedLimboResolutions;
+  private final LinkedHashSet<DocumentKey> enqueuedLimboResolutions;
 
   /** Keeps track of the target ID for each document that is in limbo with an active target. */
   private final Map<DocumentKey, Integer> activeLimboTargetsByKey;
@@ -169,7 +169,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 +603,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 +677,7 @@ private void updateTrackedLimboDocuments(List<LimboDocumentChange> 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 +695,9 @@ private void trackLimboChange(LimboDocumentChange change) {
   private void pumpEnqueuedLimboResolutions() {
     while (!enqueuedLimboResolutions.isEmpty()
         && activeLimboTargetsByKey.size() < maxConcurrentLimboResolutions) {
-      DocumentKey key = enqueuedLimboResolutions.remove();
+      Iterator<DocumentKey> it = enqueuedLimboResolutions.iterator();
+      DocumentKey key = it.next();
+      it.remove();
       int limboTargetId = targetIdGenerator.nextId();
       activeLimboResolutionsByTarget.put(limboTargetId, new LimboResolution(key));
       activeLimboTargetsByKey.put(key, limboTargetId);
@@ -714,9 +717,9 @@ public Map<DocumentKey, Integer> getActiveLimboDocumentResolutions() {
   }
 
   @VisibleForTesting
-  public Queue<DocumentKey> getEnqueuedLimboDocumentResolutions() {
-    // Make a defensive copy as the Queue continues to be modified.
-    return new ArrayDeque<>(enqueuedLimboResolutions);
+  public List<DocumentKey> getEnqueuedLimboDocumentResolutions() {
+    // Make a defensive copy as the LinkedHashSet continues to be modified.
+    return new ArrayList<>(enqueuedLimboResolutions);
   }
 
   public void handleCredentialChange(User user) {