Make notifyLocalViewChanges idempotent (#3044)

Author: schmidt-sebastian

packages/firestore/src/core/sync_engine.ts

@@ -682,8 +682,7 @@ export class SyncEngine implements RemoteSyncer {
     this.queriesByTarget.delete(targetId);
 
     if (this.isPrimaryClient) {
-      const limboKeys = this.limboDocumentRefs.referencesForId(targetId);
-      this.limboDocumentRefs.removeReferencesForId(targetId);
+      const limboKeys = this.limboDocumentRefs.removeReferencesForId(targetId);
       limboKeys.forEach(limboKey => {
         const isReferenced = this.limboDocumentRefs.containsKey(limboKey);
         if (!isReferenced) {

packages/firestore/src/local/indexeddb_mutation_queue.ts

@@ -519,7 +519,7 @@ export class IndexedDbMutationQueue implements MutationQueue {
       return PersistencePromise.forEach(
         removedDocuments,
         (key: DocumentKey) => {
-          return this.referenceDelegate.removeMutationReference(
+          return this.referenceDelegate.markPotentiallyOrphaned(
             transaction,
             key
           );

packages/firestore/src/local/indexeddb_persistence.ts

@@ -70,7 +70,6 @@ import {
   ReferenceDelegate
 } from './persistence';
 import { PersistencePromise } from './persistence_promise';
-import { ReferenceSet } from './reference_set';
 import { ClientId } from './shared_client_state';
 import { TargetData } from './target_data';
 import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
@@ -1070,8 +1069,6 @@ function clientMetadataStore(
 
 /** Provides LRU functionality for IndexedDB persistence. */
 export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
-  private inMemoryPins: ReferenceSet | null = null;
-
   readonly garbageCollector: LruGarbageCollector;
 
   constructor(private readonly db: IndexedDbPersistence, params: LruParams) {
@@ -1081,14 +1078,14 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
   getSequenceNumberCount(
     txn: PersistenceTransaction
   ): PersistencePromise<number> {
-    const docCountPromise = this.orphanedDocmentCount(txn);
+    const docCountPromise = this.orphanedDocumentCount(txn);
     const targetCountPromise = this.db.getTargetCache().getTargetCount(txn);
     return targetCountPromise.next(targetCount =>
       docCountPromise.next(docCount => targetCount + docCount)
     );
   }
 
-  private orphanedDocmentCount(
+  private orphanedDocumentCount(
     txn: PersistenceTransaction
   ): PersistencePromise<number> {
     let orphanedCount = 0;
@@ -1113,19 +1110,17 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
     );
   }
 
-  setInMemoryPins(inMemoryPins: ReferenceSet): void {
-    this.inMemoryPins = inMemoryPins;
-  }
-
   addReference(
     txn: PersistenceTransaction,
+    targetId: TargetId,
     key: DocumentKey
   ): PersistencePromise<void> {
     return writeSentinelKey(txn, key);
   }
 
   removeReference(
     txn: PersistenceTransaction,
+    targetId: TargetId,
     key: DocumentKey
   ): PersistencePromise<void> {
     return writeSentinelKey(txn, key);
@@ -1141,7 +1136,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
       .removeTargets(txn, upperBound, activeTargetIds);
   }
 
-  removeMutationReference(
+  markPotentiallyOrphaned(
     txn: PersistenceTransaction,
     key: DocumentKey
   ): PersistencePromise<void> {
@@ -1158,11 +1153,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
     txn: PersistenceTransaction,
     docKey: DocumentKey
   ): PersistencePromise<boolean> {
-    if (this.inMemoryPins!.containsKey(docKey)) {
-      return PersistencePromise.resolve<boolean>(true);
-    } else {
-      return mutationQueuesContainKey(txn, docKey);
-    }
+    return mutationQueuesContainKey(txn, docKey);
   }
 
   removeOrphanedDocuments(

packages/firestore/src/local/indexeddb_target_cache.ts

@@ -284,7 +284,7 @@ export class IndexedDbTargetCache implements TargetCache {
     keys.forEach(key => {
       const path = encodeResourcePath(key.path);
       promises.push(store.put(new DbTargetDocument(targetId, path)));
-      promises.push(this.referenceDelegate.addReference(txn, key));
+      promises.push(this.referenceDelegate.addReference(txn, targetId, key));
     });
     return PersistencePromise.waitFor(promises);
   }
@@ -301,7 +301,7 @@ export class IndexedDbTargetCache implements TargetCache {
       const path = encodeResourcePath(key.path);
       return PersistencePromise.waitFor([
         store.delete([targetId, path]),
-        this.referenceDelegate.removeReference(txn, key)
+        this.referenceDelegate.removeReference(txn, targetId, key)
       ]);
     });
   }

packages/firestore/src/local/local_store.ts

@@ -56,7 +56,6 @@ import {
 import { PersistencePromise } from './persistence_promise';
 import { TargetCache } from './target_cache';
 import { QueryEngine } from './query_engine';
-import { ReferenceSet } from './reference_set';
 import { RemoteDocumentCache } from './remote_document_cache';
 import { RemoteDocumentChangeBuffer } from './remote_document_change_buffer';
 import { ClientId } from './shared_client_state';
@@ -165,11 +164,6 @@ export class LocalStore {
    */
   protected localDocuments: LocalDocumentsView;
 
-  /**
-   * The set of document references maintained by any local views.
-   */
-  private localViewReferences = new ReferenceSet();
-
   /** Maps a target to its `TargetData`. */
   protected targetCache: TargetCache;
 
@@ -206,9 +200,6 @@ export class LocalStore {
       persistence.started,
       'LocalStore was passed an unstarted persistence implementation'
     );
-    this.persistence.referenceDelegate.setInMemoryPins(
-      this.localViewReferences
-    );
     this.mutationQueue = persistence.getMutationQueue(initialUser);
     this.remoteDocuments = persistence.getRemoteDocumentCache();
     this.targetCache = persistence.getTargetCache();
@@ -704,49 +695,56 @@ export class LocalStore {
    * Notify local store of the changed views to locally pin documents.
    */
   notifyLocalViewChanges(viewChanges: LocalViewChanges[]): Promise<void> {
-    for (const viewChange of viewChanges) {
-      const targetId = viewChange.targetId;
-
-      this.localViewReferences.addReferences(viewChange.addedKeys, targetId);
-      this.localViewReferences.removeReferences(
-        viewChange.removedKeys,
-        targetId
-      );
-
-      if (!viewChange.fromCache) {
-        const targetData = this.targetDataByTarget.get(targetId);
-        debugAssert(
-          targetData !== null,
-          `Can't set limbo-free snapshot version for unknown target: ${targetId}`
-        );
-
-        // Advance the last limbo free snapshot version
-        const lastLimboFreeSnapshotVersion = targetData.snapshotVersion;
-        const updatedTargetData = targetData.withLastLimboFreeSnapshotVersion(
-          lastLimboFreeSnapshotVersion
-        );
-        this.targetDataByTarget = this.targetDataByTarget.insert(
-          targetId,
-          updatedTargetData
-        );
-      }
-    }
-    return this.persistence.runTransaction(
-      'notifyLocalViewChanges',
-      'readwrite',
-      txn => {
+    return this.persistence
+      .runTransaction('notifyLocalViewChanges', 'readwrite', txn => {
         return PersistencePromise.forEach(
           viewChanges,
           (viewChange: LocalViewChanges) => {
             return PersistencePromise.forEach(
-              viewChange.removedKeys,
+              viewChange.addedKeys,
               (key: DocumentKey) =>
-                this.persistence.referenceDelegate.removeReference(txn, key)
+                this.persistence.referenceDelegate.addReference(
+                  txn,
+                  viewChange.targetId,
+                  key
+                )
+            ).next(() =>
+              PersistencePromise.forEach(
+                viewChange.removedKeys,
+                (key: DocumentKey) =>
+                  this.persistence.referenceDelegate.removeReference(
+                    txn,
+                    viewChange.targetId,
+                    key
+                  )
+              )
             );
           }
         );
-      }
-    );
+      })
+      .then(() => {
+        for (const viewChange of viewChanges) {
+          const targetId = viewChange.targetId;
+
+          if (!viewChange.fromCache) {
+            const targetData = this.targetDataByTarget.get(targetId);
+            debugAssert(
+              targetData !== null,
+              `Can't set limbo-free snapshot version for unknown target: ${targetId}`
+            );
+
+            // Advance the last limbo free snapshot version
+            const lastLimboFreeSnapshotVersion = targetData.snapshotVersion;
+            const updatedTargetData = targetData.withLastLimboFreeSnapshotVersion(
+              lastLimboFreeSnapshotVersion
+            );
+            this.targetDataByTarget = this.targetDataByTarget.insert(
+              targetId,
+              updatedTargetData
+            );
+          }
+        }
+      });
   }
 
   /**
@@ -869,26 +867,11 @@ export class LocalStore {
     const mode = keepPersistedTargetData ? 'readwrite' : 'readwrite-primary';
     return this.persistence
       .runTransaction('Release target', mode, txn => {
-        // References for documents sent via Watch are automatically removed
-        // when we delete a target's data from the reference delegate.
-        // Since this does not remove references for locally mutated documents,
-        // we have to remove the target associations for these documents
-        // manually.
-        // This operation needs to be run inside the transaction since EagerGC
-        // uses the local view references during the transaction's commit.
-        // Fortunately, the operation is safe to be re-run in case the
-        // transaction fails since there are no side effects if the target has
-        // already been removed.
-        const removed = this.localViewReferences.removeReferencesForId(
-          targetId
-        );
-
         if (!keepPersistedTargetData) {
-          return PersistencePromise.forEach(removed, (key: DocumentKey) =>
-            this.persistence.referenceDelegate.removeReference(txn, key)
-          ).next(() => {
-            this.persistence.referenceDelegate.removeTarget(txn, targetData!);
-          });
+          return this.persistence.referenceDelegate.removeTarget(
+            txn,
+            targetData!
+          );
         } else {
           return PersistencePromise.resolve();
         }

packages/firestore/src/local/memory_mutation_queue.ts

@@ -299,7 +299,7 @@ export class MemoryMutationQueue implements MutationQueue {
     return PersistencePromise.forEach(batch.mutations, (mutation: Mutation) => {
       const ref = new DocReference(mutation.key, batch.batchId);
       references = references.delete(ref);
-      return this.referenceDelegate.removeMutationReference(
+      return this.referenceDelegate.markPotentiallyOrphaned(
         transaction,
         mutation.key
       );