Implement code review feedback from @wilhuff, round 2

dconeybe · dconeybe · commit f0a20779a774 · 2020-03-30T11:51:12.000-04:00
diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts
@@ -148,14 +148,23 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     q.canonicalId()
   );
   private queriesByTarget: { [targetId: number]: Query[] } = {};
-  /** The keys of documents that are in limbo for which we haven't yet started a limbo resolution query. */
-  private limboListenQueue: DocumentKey[] = [];
-  /** Keeps track of the target ID for each document that is in limbo with an active target. */
-  private limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
+  /**
+   * The keys of documents that are in limbo for which we haven't yet started a
+   * limbo resolution query.
+   */
+  private enqueuedLimboResolutions: DocumentKey[] = [];
+  /**
+   * Keeps track of the target ID for each document that is in limbo with an
+   * active target.
+   */
+  private activeLimboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
     DocumentKey.comparator
   );
-  /** Keeps track of the information about an active limbo resolution for each active target ID that was started for the purpose of limbo resolution. */
-  private limboResolutionsByTarget: {
+  /**
+   * Keeps track of the information about an active limbo resolution for each
+   * active target ID that was started for the purpose of limbo resolution.
+   */
+  private activeLimboResolutionsByTarget: {
     [targetId: number]: LimboResolution;
   } = {};
   private limboDocumentRefs = new ReferenceSet();
@@ -405,7 +414,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       const changes = await this.localStore.applyRemoteEvent(remoteEvent);
       // Update `receivedDocument` as appropriate for any limbo targets.
       objUtils.forEach(remoteEvent.targetChanges, (targetId, targetChange) => {
-        const limboResolution = this.limboResolutionsByTarget[Number(targetId)];
+        const limboResolution = this.activeLimboResolutionsByTarget[
+          Number(targetId)
+        ];
         if (limboResolution) {
           // Since this is a limbo resolution lookup, it's for a single document
           // and it could be added, modified, or removed, but not a combination.
@@ -484,14 +495,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     // PORTING NOTE: Multi-tab only.
     this.sharedClientState.updateQueryState(targetId, 'rejected', err);
 
-    const limboResolution = this.limboResolutionsByTarget[targetId];
+    const limboResolution = this.activeLimboResolutionsByTarget[targetId];
     const limboKey = limboResolution && limboResolution.key;
     if (limboKey) {
       // Since this query failed, we won't want to manually unlisten to it.
       // So go ahead and remove it from bookkeeping.
-      this.limboTargetsByKey = this.limboTargetsByKey.remove(limboKey);
-      delete this.limboResolutionsByTarget[targetId];
-      this.pumpLimboResolutionListenQueue();
+      this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.remove(
+        limboKey
+      );
+      delete this.activeLimboResolutionsByTarget[targetId];
+      this.pumpEnqueuedLimboResolutions();
 
       // TODO(klimt): We really only should do the following on permission
       // denied errors, but we don't have the cause code here.
@@ -737,16 +750,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
   private removeLimboTarget(key: DocumentKey): void {
     // 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.
-    const limboTargetId = this.limboTargetsByKey.get(key);
+    const limboTargetId = this.activeLimboTargetsByKey.get(key);
     if (limboTargetId === null) {
       // This target already got removed, because the query failed.
       return;
     }
 
     this.remoteStore.unlisten(limboTargetId);
-    this.limboTargetsByKey = this.limboTargetsByKey.remove(key);
-    delete this.limboResolutionsByTarget[limboTargetId];
-    this.pumpLimboResolutionListenQueue();
+    this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.remove(key);
+    delete this.activeLimboResolutionsByTarget[limboTargetId];
+    this.pumpEnqueuedLimboResolutions();
   }
 
   private updateTrackedLimbos(
@@ -775,41 +788,32 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
   private trackLimboChange(limboChange: AddedLimboDocument): void {
     const key = limboChange.key;
-    if (!this.limboTargetsByKey.get(key)) {
+    if (!this.activeLimboTargetsByKey.get(key)) {
       log.debug(LOG_TAG, 'New document in limbo: ' + key);
-      this.limboListenQueue.push(key);
-      this.pumpLimboResolutionListenQueue();
+      this.enqueuedLimboResolutions.push(key);
+      this.pumpEnqueuedLimboResolutions();
     }
   }
 
   /**
-   * Starts listens for documents in limbo that are enqueued for resolution.
-   *
-   * When a document goes into limbo it is enqueued for resolution. This method
-   * repeatedly removes entries from the limbo resolution queue and starts a
-   * listen for them until either (1) the queue is empty, meaning that all
-   * documents that were in limbo either have active listens or have been
-   * resolved, or (2) the maximum number of concurrent limbo resolution listens
-   * has been reached.
+   * Starts listens for documents in limbo that are enqueued for resolution,
+   * subject to a maximum number of concurrent resolutions.
    *
-   * This method is invoked every time an entry is added to the limbo
-   * resolution queue and every time that a limbo resolution listen completes
-   * (either successfully or unsuccessfully). This ensures that all documents in
-   * limbo are eventually resolved.
-   *
-   * A maximum number of concurrent limbo resolution listens was implemented to
-   * prevent an unbounded number of active limbo resolution listens that can
-   * exhaust server resources and result in "resource exhausted" errors.
+   * Without bounding the number of concurrent resolutions, the server can fail
+   * with "resource exhausted" errors which can lead to pathological client
+   * behavior as seen in https://github.com/firebase/firebase-js-sdk/issues/2683.
    */
-  private pumpLimboResolutionListenQueue(): void {
+  private pumpEnqueuedLimboResolutions(): void {
     while (
-      this.limboListenQueue.length > 0 &&
-      this.limboTargetsByKey.size < this.maxConcurrentLimboResolutions
+      this.enqueuedLimboResolutions.length > 0 &&
+      this.activeLimboTargetsByKey.size < this.maxConcurrentLimboResolutions
     ) {
-      const key = this.limboListenQueue.shift()!;
+      const key = this.enqueuedLimboResolutions.shift()!;
       const limboTargetId = this.limboTargetIdGenerator.next();
-      this.limboResolutionsByTarget[limboTargetId] = new LimboResolution(key);
-      this.limboTargetsByKey = this.limboTargetsByKey.insert(
+      this.activeLimboResolutionsByTarget[limboTargetId] = new LimboResolution(
+        key
+      );
+      this.activeLimboTargetsByKey = this.activeLimboTargetsByKey.insert(
         key,
         limboTargetId
       );
@@ -826,12 +830,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
   // Visible for testing
   activeLimboDocumentResolutions(): SortedMap<DocumentKey, TargetId> {
-    return this.limboTargetsByKey;
+    return this.activeLimboTargetsByKey;
   }
 
   // Visible for testing
-  documentsEnqueuedForLimboResolution(): DocumentKey[] {
-    return this.limboListenQueue;
+  enqueuedLimboDocumentResolutions(): DocumentKey[] {
+    return this.enqueuedLimboResolutions;
   }
 
   private async emitNewSnapsAndNotifyLocalStore(
@@ -977,12 +981,12 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
 
   // PORTING NOTE: Multi-tab only.
   private resetLimboDocuments(): void {
-    objUtils.forEachNumber(this.limboResolutionsByTarget, targetId => {
+    objUtils.forEachNumber(this.activeLimboResolutionsByTarget, targetId => {
       this.remoteStore.unlisten(targetId);
     });
     this.limboDocumentRefs.removeAllReferences();
-    this.limboResolutionsByTarget = [];
-    this.limboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
+    this.activeLimboResolutionsByTarget = [];
+    this.activeLimboTargetsByKey = new SortedMap<DocumentKey, TargetId>(
       DocumentKey.comparator
     );
   }
@@ -1179,7 +1183,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
   }
 
   getRemoteKeysForTarget(targetId: TargetId): DocumentKeySet {
-    const limboResolution = this.limboResolutionsByTarget[targetId];
+    const limboResolution = this.activeLimboResolutionsByTarget[targetId];
     if (limboResolution && limboResolution.receivedDocument) {
       return documentKeySet().add(limboResolution.key);
     } else {
diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts
@@ -674,7 +674,7 @@ describeSpec('Limbo Documents:', [], () => {
           .watchCurrents(query, 'resume-token-2000')
           .watchSnapshots(2000)
           .expectLimboDocs(doc1.key, doc2.key)
-          .expectInactiveLimboDocs(doc3.key, doc4.key, doc5.key)
+          .expectEnqueuedLimboDocs(doc3.key, doc4.key, doc5.key)
           // Limbo document causes query to be "inconsistent"
           .expectEvents(query, { fromCache: true })
           .watchAcks(limboQuery1)
@@ -689,7 +689,7 @@ describeSpec('Limbo Documents:', [], () => {
           })
           // Start the second round of limbo resolutions.
           .expectLimboDocs(doc3.key, doc4.key)
-          .expectInactiveLimboDocs(doc5.key)
+          .expectEnqueuedLimboDocs(doc5.key)
           .watchAcks(limboQuery3)
           .watchAcks(limboQuery4)
           // Resolve limbo documents doc3 and doc4 in a single snapshot.
@@ -702,7 +702,7 @@ describeSpec('Limbo Documents:', [], () => {
           })
           // Start the final round of limbo resolutions.
           .expectLimboDocs(doc5.key)
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
           .watchAcks(limboQuery5)
           // Resolve limbo document doc5.
           .watchCurrents(limboQuery5, 'resume-token-2003')
@@ -712,7 +712,7 @@ describeSpec('Limbo Documents:', [], () => {
             fromCache: false
           })
           .expectLimboDocs()
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
       );
     }
   );
@@ -751,7 +751,7 @@ describeSpec('Limbo Documents:', [], () => {
           .watchCurrents(query, 'resume-token-2000')
           .watchSnapshots(2000)
           .expectLimboDocs(doc1.key, doc2.key)
-          .expectInactiveLimboDocs(doc3.key, doc4.key, doc5.key)
+          .expectEnqueuedLimboDocs(doc3.key, doc4.key, doc5.key)
           // Limbo document causes query to be "inconsistent"
           .expectEvents(query, { fromCache: true })
           .watchAcks(limboQuery1)
@@ -762,37 +762,37 @@ describeSpec('Limbo Documents:', [], () => {
           .expectEvents(query, { removed: [doc1], fromCache: true })
           // Start the next limbo resolution since one has finished.
           .expectLimboDocs(doc2.key, doc3.key)
-          .expectInactiveLimboDocs(doc4.key, doc5.key)
+          .expectEnqueuedLimboDocs(doc4.key, doc5.key)
           .watchAcks(limboQuery3)
           // Resolve the limbo documents doc2 in its own snapshot.
           .watchCurrents(limboQuery2, 'resume-token-2002')
           .watchSnapshots(2002)
           .expectEvents(query, { removed: [doc2], fromCache: true })
           // Start the next limbo resolution since one has finished.
           .expectLimboDocs(doc3.key, doc4.key)
-          .expectInactiveLimboDocs(doc5.key)
+          .expectEnqueuedLimboDocs(doc5.key)
           .watchAcks(limboQuery4)
           // Resolve the limbo documents doc3 in its own snapshot.
           .watchCurrents(limboQuery3, 'resume-token-2003')
           .watchSnapshots(2003)
           .expectEvents(query, { removed: [doc3], fromCache: true })
           // Start the next limbo resolution since one has finished.
           .expectLimboDocs(doc4.key, doc5.key)
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
           .watchAcks(limboQuery5)
           // Resolve the limbo documents doc4 in its own snapshot.
           .watchCurrents(limboQuery4, 'resume-token-2004')
           .watchSnapshots(2004)
           .expectEvents(query, { removed: [doc4], fromCache: true })
           // The final limbo document listen is already active; resolve it.
           .expectLimboDocs(doc5.key)
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
           // Resolve the limbo documents doc5 in its own snapshot.
           .watchCurrents(limboQuery5, 'resume-token-2005')
           .watchSnapshots(2005)
           .expectEvents(query, { removed: [doc5], fromCache: false })
           .expectLimboDocs()
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
       );
     }
   );
@@ -823,7 +823,7 @@ describeSpec('Limbo Documents:', [], () => {
           .watchCurrents(query, 'resume-token-1001')
           .watchSnapshots(2000)
           .expectLimboDocs(doc1.key)
-          .expectInactiveLimboDocs(doc2.key)
+          .expectEnqueuedLimboDocs(doc2.key)
           // Limbo document causes query to be "inconsistent"
           .expectEvents(query, { fromCache: true })
           .watchRemoves(
@@ -835,7 +835,7 @@ describeSpec('Limbo Documents:', [], () => {
           // start.
           .expectEvents(query, { removed: [doc1], fromCache: true })
           .expectLimboDocs(doc2.key)
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
           // Reject the listen for the second limbo resolution as well, in order
           // to exercise the code path of a rejected limbo resolution without
           // any enqueued limbo resolutions.
@@ -845,7 +845,7 @@ describeSpec('Limbo Documents:', [], () => {
           )
           .expectEvents(query, { removed: [doc2] })
           .expectLimboDocs()
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
       );
     }
   );
@@ -910,21 +910,21 @@ describeSpec('Limbo Documents:', [], () => {
           .watchAcksFull(query, 1002, docB1, docB2, docB3)
           // The docAs are now in limbo; the client begins limbo resolution.
           .expectLimboDocs(docA1.key, docA2.key)
-          .expectInactiveLimboDocs(docA3.key)
+          .expectEnqueuedLimboDocs(docA3.key)
           .watchAcks(docA1Query)
           .watchAcks(docA2Query)
           .watchCurrents(docA1Query, 'resume-token-1003')
           .watchCurrents(docA2Query, 'resume-token-1003')
           .watchSnapshots(1003)
           .expectEvents(query, { removed: [docA1, docA2], fromCache: true })
           .expectLimboDocs(docA3.key)
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
           .watchAcks(docA3Query)
           .watchCurrents(docA3Query, 'resume-token-1004')
           .watchSnapshots(1004)
           .expectEvents(query, { removed: [docA3] })
           .expectLimboDocs()
-          .expectInactiveLimboDocs()
+          .expectEnqueuedLimboDocs()
       );
     }
   );
diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts
@@ -359,7 +359,7 @@ export class SpecBuilder {
       expectedState: {
         activeTargets: {},
         activeLimboDocs: [],
-        inactiveLimboDocs: []
+        enqueuedLimboDocs: []
       }
     };
     return this;
@@ -388,7 +388,7 @@ export class SpecBuilder {
       expectedState: {
         activeTargets: {},
         activeLimboDocs: [],
-        inactiveLimboDocs: []
+        enqueuedLimboDocs: []
       }
     };
     // Reset our mappings / target ids since all existing listens will be
@@ -404,7 +404,7 @@ export class SpecBuilder {
       expectedState: {
         activeTargets: {},
         activeLimboDocs: [],
-        inactiveLimboDocs: []
+        enqueuedLimboDocs: []
       }
     };
     // Reset our mappings / target ids since all existing listens will be
@@ -477,14 +477,15 @@ export class SpecBuilder {
   }
 
   /**
-   * Expects a document to be in limbo, but *without* a targetId.
+   * Expects a document to be in limbo, enqueued for limbo resolution, and
+   * therefore *without* an active targetId.
    */
-  expectInactiveLimboDocs(...keys: DocumentKey[]): this {
+  expectEnqueuedLimboDocs(...keys: DocumentKey[]): this {
     this.assertStep('Limbo expectation requires previous step');
     const currentStep = this.currentStep!;
 
     currentStep.expectedState = currentStep.expectedState || {};
-    currentStep.expectedState.inactiveLimboDocs = keys.map(k =>
+    currentStep.expectedState.enqueuedLimboDocs = keys.map(k =>
       SpecBuilder.keyToSpec(k)
     );
 
diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts
