Fixes #1436: Ensure only the primary tab tries to garbage collect. (#1441)

mikelehen · web-flow · commit 0bbf7088d767 · 2018-12-23T10:41:43.000-08:00
Only schedule garbage collections on the primary tab and gracefully ignore
if they fail.
diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md
@@ -1,4 +1,12 @@
 # Unreleased
+- [fixed] Fixed a regression introduced in 5.7.0 that caused apps using
+  experimentalTabSynchronization to hit an exception for "Failed to obtain
+  primary lease for action 'Collect garbage'".
+
+# 0.9.1
+- [changed] Added a custom error for schema downgrades.
+
+# 0.9.0
 - [changed] Removed eval()-based fallback for JSON parsing, allowing SDK to
   be used in environments that prohibit eval().
 - [feature] Added a garbage collection process to on-disk persistence that
diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts
@@ -393,7 +393,6 @@ export class FirestoreClient {
             this.asyncQueue,
             this.localStore
           );
-          this.lruScheduler.start();
         }
         const serializer = this.platform.newSerializer(
           this.databaseInfo.databaseId
@@ -444,9 +443,16 @@ export class FirestoreClient {
 
         // NOTE: This will immediately call the listener, so we make sure to
         // set it after localStore / remoteStore are started.
-        await this.persistence.setPrimaryStateListener(isPrimary =>
-          this.syncEngine.applyPrimaryState(isPrimary)
-        );
+        await this.persistence.setPrimaryStateListener(async isPrimary => {
+          await this.syncEngine.applyPrimaryState(isPrimary);
+          if (this.lruScheduler) {
+            if (isPrimary && !this.lruScheduler.started) {
+              this.lruScheduler.start();
+            } else if (!isPrimary) {
+              this.lruScheduler.stop();
+            }
+          }
+        });
       });
   }
 
diff --git a/packages/firestore/src/core/sync_engine.ts b/packages/firestore/src/core/sync_engine.ts
@@ -40,7 +40,7 @@ import { Deferred } from '../util/promise';
 import { SortedMap } from '../util/sorted_map';
 import { isNullOrUndefined } from '../util/types';
 
-import { isPrimaryLeaseLostError } from '../local/indexeddb_persistence';
+import { ignoreIfPrimaryLeaseLoss } from '../local/indexeddb_persistence';
 import { isDocumentChangeMissingError } from '../local/indexeddb_remote_document_cache';
 import { ClientId, SharedClientState } from '../local/shared_client_state';
 import {
@@ -324,7 +324,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
             this.remoteStore.unlisten(queryView.targetId);
             this.removeAndCleanupQuery(queryView);
           })
-          .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+          .catch(ignoreIfPrimaryLeaseLoss);
       }
     } else {
       this.removeAndCleanupQuery(queryView);
@@ -464,7 +464,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
         );
         return this.emitNewSnapsAndNotifyLocalStore(changes, remoteEvent);
       })
-      .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+      .catch(ignoreIfPrimaryLeaseLoss);
   }
 
   /**
@@ -547,7 +547,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
       await this.localStore
         .releaseQuery(queryView.query, /* keepPersistedQueryData */ false)
         .then(() => this.removeAndCleanupQuery(queryView))
-        .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+        .catch(ignoreIfPrimaryLeaseLoss);
       this.syncEngineListener!.onWatchError(queryView.query, err);
     }
   }
@@ -609,7 +609,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
         this.sharedClientState.updateMutationState(batchId, 'acknowledged');
         return this.emitNewSnapsAndNotifyLocalStore(changes);
       })
-      .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+      .catch(ignoreIfPrimaryLeaseLoss);
   }
 
   rejectFailedWrite(batchId: BatchId, error: FirestoreError): Promise<void> {
@@ -627,7 +627,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
         this.sharedClientState.updateMutationState(batchId, 'rejected', error);
         return this.emitNewSnapsAndNotifyLocalStore(changes);
       })
-      .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+      .catch(ignoreIfPrimaryLeaseLoss);
   }
 
   private addMutationCallback(
@@ -815,24 +815,6 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
     await this.localStore.notifyLocalViewChanges(docChangesInAllViews);
   }
 
-  /**
-   * Verifies the error thrown by an LocalStore operation. If a LocalStore
-   * operation fails because the primary lease has been taken by another client,
-   * we ignore the error (the persistence layer will immediately call
-   * `applyPrimaryLease` to propagate the primary state change). All other
-   * errors are re-thrown.
-   *
-   * @param err An error returned by a LocalStore operation.
-   * @return A Promise that resolves after we recovered, or the original error.
-   */
-  private async ignoreIfPrimaryLeaseLoss(err: FirestoreError): Promise<void> {
-    if (isPrimaryLeaseLostError(err)) {
-      log.debug(LOG_TAG, 'Unexpectedly lost primary lease');
-    } else {
-      throw err;
-    }
-  }
-
   private assertSubscribed(fnName: string): void {
     assert(
       this.syncEngineListener !== null,
@@ -1068,7 +1050,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
             this.remoteStore.unlisten(targetId);
             this.removeAndCleanupQuery(queryView);
           })
-          .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+          .catch(ignoreIfPrimaryLeaseLoss);
       }
     }
   }
diff --git a/packages/firestore/src/local/indexeddb_persistence.ts b/packages/firestore/src/local/indexeddb_persistence.ts
@@ -1045,12 +1045,33 @@ export class IndexedDbPersistence implements Persistence {
   }
 }
 
-export function isPrimaryLeaseLostError(err: FirestoreError): boolean {
+function isPrimaryLeaseLostError(err: FirestoreError): boolean {
   return (
     err.code === Code.FAILED_PRECONDITION &&
     err.message === PRIMARY_LEASE_LOST_ERROR_MSG
   );
 }
+
+/**
+ * Verifies the error thrown by a LocalStore operation. If a LocalStore
+ * operation fails because the primary lease has been taken by another client,
+ * we ignore the error (the persistence layer will immediately call
+ * `applyPrimaryLease` to propagate the primary state change). All other errors
+ * are re-thrown.
+ *
+ * @param err An error returned by a LocalStore operation.
+ * @return A Promise that resolves after we recovered, or the original error.
+ */
+export async function ignoreIfPrimaryLeaseLoss(
+  err: FirestoreError
+): Promise<void> {
+  if (isPrimaryLeaseLostError(err)) {
+    log.debug(LOG_TAG, 'Unexpectedly lost primary lease');
+  } else {
+    throw err;
+  }
+}
+
 /**
  * Helper to get a typed SimpleDbStore for the primary client object store.
  */
diff --git a/packages/firestore/src/local/lru_garbage_collector.ts b/packages/firestore/src/local/lru_garbage_collector.ts
@@ -23,6 +23,7 @@ import * as log from '../util/log';
 import { AnyJs, primitiveComparator } from '../util/misc';
 import { CancelablePromise } from '../util/promise';
 import { SortedSet } from '../util/sorted_set';
+import { ignoreIfPrimaryLeaseLoss } from './indexeddb_persistence';
 import { LocalStore } from './local_store';
 import { PersistenceTransaction } from './persistence';
 import { PersistencePromise } from './persistence_promise';
@@ -247,6 +248,10 @@ export class LruScheduler {
     }
   }
 
+  get started(): boolean {
+    return this.gcTask !== null;
+  }
+
   private scheduleGC(): void {
     assert(this.gcTask === null, 'Cannot schedule GC while a task is pending');
     const delay = this.hasRun ? REGULAR_GC_DELAY_MS : INITIAL_GC_DELAY_MS;
@@ -262,7 +267,8 @@ export class LruScheduler {
         this.hasRun = true;
         return this.localStore
           .collectGarbage(this.garbageCollector)
-          .then(() => this.scheduleGC());
+          .then(() => this.scheduleGC())
+          .catch(ignoreIfPrimaryLeaseLoss);
       }
     );
   }
diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts
@@ -31,7 +31,7 @@ import { Code, FirestoreError } from '../util/error';
 import * as log from '../util/log';
 import * as objUtils from '../util/obj';
 
-import { isPrimaryLeaseLostError } from '../local/indexeddb_persistence';
+import { ignoreIfPrimaryLeaseLoss } from '../local/indexeddb_persistence';
 import { DocumentKeySet } from '../model/collections';
 import { AsyncQueue } from '../util/async_queue';
 import { Datastore } from './datastore';
@@ -564,23 +564,7 @@ export class RemoteStore implements TargetMetadataProvider {
           this.writeStream.writeMutations(batch.mutations);
         }
       })
-      .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
-  }
-
-  /**
-   * Verifies the error thrown by an LocalStore operation. If a LocalStore
-   * operation fails because the primary lease has been taken by another client,
-   * we ignore the error. All other errors are re-thrown.
-   *
-   * @param err An error returned by a LocalStore operation.
-   * @return A Promise that resolves after we recovered, or the original error.
-   */
-  private ignoreIfPrimaryLeaseLoss(err: FirestoreError): void {
-    if (isPrimaryLeaseLostError(err)) {
-      log.debug(LOG_TAG, 'Unexpectedly lost primary lease');
-    } else {
-      throw err;
-    }
+      .catch(ignoreIfPrimaryLeaseLoss);
   }
 
   private onMutationResult(
@@ -656,7 +640,7 @@ export class RemoteStore implements TargetMetadataProvider {
 
       return this.localStore
         .setLastStreamToken(emptyByteString())
-        .catch(err => this.ignoreIfPrimaryLeaseLoss(err));
+        .catch(ignoreIfPrimaryLeaseLoss);
     } else {
       // Some other error, don't reset stream token. Our stream logic will
       // just retry with exponential backoff.
