Make shutdown idempotent (#3575)

Author: schmidt-sebastian

.changeset/neat-gorillas-behave.md

@@ -0,0 +1,5 @@
+---
+"@firebase/firestore": patch
+---
+
+`terminate()` can now be retried if it fails with an IndexedDB exception.

packages/firestore/exp/src/api/database.ts

@@ -27,7 +27,10 @@ import {
   enqueueWaitForPendingWrites,
   MAX_CONCURRENT_LIMBO_RESOLUTIONS
 } from '../../../src/core/firestore_client';
-import { AsyncQueue } from '../../../src/util/async_queue';
+import {
+  AsyncQueue,
+  wrapInUserErrorIfRecoverable
+} from '../../../src/util/async_queue';
 import {
   ComponentConfiguration,
   IndexedDbOfflineComponentProvider,
@@ -59,7 +62,6 @@ import { AutoId } from '../../../src/util/misc';
 import { User } from '../../../src/auth/user';
 import { CredentialChangeListener } from '../../../src/api/credentials';
 import { logDebug } from '../../../src/util/log';
-import { debugAssert } from '../../../src/util/assert';
 
 const LOG_TAG = 'Firestore';
 
@@ -131,16 +133,28 @@ export class Firestore extends LiteFirestore
   }
 
   _terminate(): Promise<void> {
-    debugAssert(!this._terminated, 'Cannot invoke _terminate() more than once');
-    return this._queue.enqueueAndInitiateShutdown(async () => {
-      await super._terminate();
-      await removeComponents(this);
-
-      // `removeChangeListener` must be called after shutting down the
-      // RemoteStore as it will prevent the RemoteStore from retrieving
-      // auth tokens.
-      this._credentials.removeChangeListener();
+    this._queue.enterRestrictedMode();
+    const deferred = new Deferred();
+    this._queue.enqueueAndForgetEvenWhileRestricted(async () => {
+      try {
+        await super._terminate();
+        await removeComponents(this);
+
+        // `removeChangeListener` must be called after shutting down the
+        // RemoteStore as it will prevent the RemoteStore from retrieving
+        // auth tokens.
+        this._credentials.removeChangeListener();
+
+        deferred.resolve();
+      } catch (e) {
+        const firestoreError = wrapInUserErrorIfRecoverable(
+          e,
+          `Failed to shutdown persistence`
+        );
+        deferred.reject(firestoreError);
+      }
     });
+    return deferred.promise;
   }
 }
 
@@ -242,7 +256,7 @@ export function clearIndexedDbPersistence(
   }
 
   const deferred = new Deferred<void>();
-  firestoreImpl._queue.enqueueAndForgetEvenAfterShutdown(async () => {
+  firestoreImpl._queue.enqueueAndForgetEvenWhileRestricted(async () => {
     try {
       await indexedDbClearPersistence(
         indexedDbStoragePrefix(

packages/firestore/src/api/credentials.ts

@@ -127,10 +127,6 @@ export class EmptyCredentialsProvider implements CredentialsProvider {
   }
 
   removeChangeListener(): void {
-    debugAssert(
-      this.changeListener !== null,
-      'removeChangeListener() when no listener registered'
-    );
     this.changeListener = null;
   }
 }
@@ -252,15 +248,6 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
   }
 
   removeChangeListener(): void {
-    debugAssert(
-      this.tokenListener != null,
-      'removeChangeListener() called twice'
-    );
-    debugAssert(
-      this.changeListener !== null,
-      'removeChangeListener() called when no listener registered'
-    );
-
     if (this.auth) {
       this.auth.removeAuthTokenListener(this.tokenListener!);
     }

packages/firestore/src/api/database.ts

@@ -448,7 +448,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
     }
 
     const deferred = new Deferred<void>();
-    this._queue.enqueueAndForgetEvenAfterShutdown(async () => {
+    this._queue.enqueueAndForgetEvenWhileRestricted(async () => {
       try {
         await this._offlineComponentProvider.clearPersistence(
           this._databaseId,

packages/firestore/src/core/firestore_client.ts

@@ -363,21 +363,33 @@ export class FirestoreClient {
   }
 
   terminate(): Promise<void> {
-    return this.asyncQueue.enqueueAndInitiateShutdown(async () => {
-      // PORTING NOTE: LocalStore does not need an explicit shutdown on web.
-      if (this.gcScheduler) {
-        this.gcScheduler.stop();
-      }
-
-      await this.remoteStore.shutdown();
-      await this.sharedClientState.shutdown();
-      await this.persistence.shutdown();
+    this.asyncQueue.enterRestrictedMode();
+    const deferred = new Deferred();
+    this.asyncQueue.enqueueAndForgetEvenWhileRestricted(async () => {
+      try {
+        // PORTING NOTE: LocalStore does not need an explicit shutdown on web.
+        if (this.gcScheduler) {
+          this.gcScheduler.stop();
+        }
 
-      // `removeChangeListener` must be called after shutting down the
-      // RemoteStore as it will prevent the RemoteStore from retrieving
-      // auth tokens.
-      this.credentials.removeChangeListener();
+        await this.remoteStore.shutdown();
+        await this.sharedClientState.shutdown();
+        await this.persistence.shutdown();
+
+        // `removeChangeListener` must be called after shutting down the
+        // RemoteStore as it will prevent the RemoteStore from retrieving
+        // auth tokens.
+        this.credentials.removeChangeListener();
+        deferred.resolve();
+      } catch (e) {
+        const firestoreError = wrapInUserErrorIfRecoverable(
+          e,
+          `Failed to shutdown persistence`
+        );
+        deferred.reject(firestoreError);
+      }
     });
+    return deferred.promise;
   }
 
   /**

packages/firestore/src/local/indexeddb_persistence.ts

@@ -680,13 +680,22 @@ export class IndexedDbPersistence implements Persistence {
     }
     this.detachVisibilityHandler();
     this.detachWindowUnloadHook();
-    await this.runTransaction('shutdown', 'readwrite', txn => {
-      return this.releasePrimaryLeaseIfHeld(txn).next(() =>
-        this.removeClientMetadata(txn)
-      );
-    }).catch(e => {
-      logDebug(LOG_TAG, 'Proceeding with shutdown despite failure: ', e);
-    });
+
+    // Use `SimpleDb.runTransaction` directly to avoid failing if another tab
+    // has obtained the primary lease.
+    await this.simpleDb.runTransaction(
+      'readwrite',
+      [DbPrimaryClient.store, DbClientMetadata.store],
+      simpleDbTxn => {
+        const persistenceTransaction = new IndexedDbTransaction(
+          simpleDbTxn,
+          ListenSequence.INVALID
+        );
+        return this.releasePrimaryLeaseIfHeld(persistenceTransaction).next(() =>
+          this.removeClientMetadata(persistenceTransaction)
+        );
+      }
+    );
     this.simpleDb.close();
 
     // Remove the entry marking the client as zombied from LocalStorage since

packages/firestore/src/util/async_queue.ts

@@ -273,7 +273,7 @@ export class AsyncQueue {
    * Regardless if the queue has initialized shutdown, adds a new operation to the
    * queue without waiting for it to complete (i.e. we ignore the Promise result).
    */
-  enqueueAndForgetEvenAfterShutdown<T extends unknown>(
+  enqueueAndForgetEvenWhileRestricted<T extends unknown>(
     op: () => Promise<T>
   ): void {
     this.verifyNotFailed();
@@ -282,25 +282,11 @@ export class AsyncQueue {
   }
 
   /**
-   * Regardless if the queue has initialized shutdown, adds a new operation to the
-   * queue.
+   * Initialize the shutdown of this queue. Once this method is called, the
+   * only possible way to request running an operation is through
+   * `enqueueEvenWhileRestricted()`.
    */
-  private enqueueEvenAfterShutdown<T extends unknown>(
-    op: () => Promise<T>
-  ): Promise<T> {
-    this.verifyNotFailed();
-    return this.enqueueInternal(op);
-  }
-
-  /**
-   * Adds a new operation to the queue and initialize the shut down of this queue.
-   * Returns a promise that will be resolved when the promise returned by the new
-   * operation is (with its value).
-   * Once this method is called, the only possible way to request running an operation
-   * is through `enqueueAndForgetEvenAfterShutdown`.
-   */
-  async enqueueAndInitiateShutdown(op: () => Promise<void>): Promise<void> {
-    this.verifyNotFailed();
+  enterRestrictedMode(): void {
     if (!this._isShuttingDown) {
       this._isShuttingDown = true;
       const document = getDocument();
@@ -310,7 +296,6 @@ export class AsyncQueue {
           this.visibilityHandler
         );
       }
-      await this.enqueueEvenAfterShutdown(op);
     }
   }
 

packages/firestore/test/integration/api/database.test.ts

@@ -1079,6 +1079,13 @@ apiDescribe('Database', (persistence: boolean) => {
     });
   });
 
+  it('can call terminate() multiple times', async () => {
+    return withTestDb(persistence, async db => {
+      await db.terminate();
+      await db.terminate();
+    });
+  });
+
   // eslint-disable-next-line no-restricted-properties
   (MEMORY_ONLY_BUILD ? it : it.skip)(
     'recovers when persistence is missing',

packages/firestore/test/integration/util/firebase_export.ts

@@ -80,9 +80,6 @@ export function newTestFirestore(
       : getFirestore(app);
     return new FirestoreShim(firestore);
   } else {
-    if (nameOrApp === undefined) {
-      nameOrApp = 'test-app-' + appCount++;
-    }
     const app =
       typeof nameOrApp === 'string'
         ? firebase.initializeApp(

packages/firestore/test/unit/local/indexeddb_persistence.test.ts

@@ -1182,6 +1182,7 @@ describe('IndexedDb: allowTabSynchronization', () => {
         await expect(db1.start()).to.eventually.be.rejectedWith(
           'IndexedDB transaction failed'
         );
+        await db1.shutdown();
       }
     );
   });

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -299,11 +299,15 @@ abstract class TestRunner {
   }
 
   async shutdown(): Promise<void> {
-    await this.queue.enqueueAndInitiateShutdown(async () => {
+    this.queue.enterRestrictedMode();
+    const deferred = new Deferred();
+    this.queue.enqueueAndForgetEvenWhileRestricted(async () => {
       if (this.started) {
         await this.doShutdown();
       }
+      deferred.resolve();
     });
+    return deferred.promise;
   }
 
   /** Runs a single SpecStep on this runner. */

packages/firestore/test/unit/util/async_queue.test.ts

@@ -396,11 +396,11 @@ describe('AsyncQueue', () => {
     queue.enqueueAndForget(() => doStep(1));
 
     // After this call, only operations requested via
-    // `enqueueAndForgetEvenAfterShutdown` gets executed.
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    queue.enqueueAndInitiateShutdown(() => doStep(2));
+    // `enqueueAndForgetEvenWhileRestricted` gets executed.
+    queue.enterRestrictedMode();
+    queue.enqueueAndForgetEvenWhileRestricted(() => doStep(2));
     queue.enqueueAndForget(() => doStep(3));
-    queue.enqueueAndForgetEvenAfterShutdown(() => doStep(4));
+    queue.enqueueAndForgetEvenWhileRestricted(() => doStep(4));
 
     await queue.drain();
     expect(completedSteps).to.deep.equal([1, 2, 4]);