Make shutdown idempotent

Author: schmidt-sebastian

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.initiateShutdown();
+    const deferred = new Deferred();
+    this._queue.enqueueAndForgetEvenAfterShutdown(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;
   }
 }
 

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/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.initiateShutdown();
+    const deferred = new Deferred();
+    this.asyncQueue.enqueueAndForgetEvenAfterShutdown(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

@@ -684,8 +684,6 @@ export class IndexedDbPersistence implements Persistence {
       return this.releasePrimaryLeaseIfHeld(txn).next(() =>
         this.removeClientMetadata(txn)
       );
-    }).catch(e => {
-      logDebug(LOG_TAG, 'Proceeding with shutdown despite failure: ', e);
     });
     this.simpleDb.close();
 

packages/firestore/src/util/async_queue.ts

@@ -272,32 +272,17 @@ 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
+   * `enqueueAndForgetEvenAfterShutdown`.
    */
-  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();
+  initiateShutdown(): void {
     if (!this._isShuttingDown) {
       this._isShuttingDown = true;
       const window = getWindow();
       if (window) {
         window.removeEventListener('visibilitychange', 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/unit/specs/recovery_spec.test.ts

@@ -840,4 +840,12 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       })
       .userUnlistens(query1);
   });
+  
+  specTest('Terminate (with recovery)', [], () => {
+    return spec()
+      .failDatabaseTransactions('shutdown')
+      .shutdown({ expectFailure: true })
+      .recoverDatabase()
+      .shutdown();
+  });
 });

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

@@ -436,10 +436,10 @@ export class SpecBuilder {
     return this;
   }
 
-  shutdown(): this {
+  shutdown(options?: { expectFailure?: boolean }): this {
     this.nextStep();
     this.currentStep = {
-      shutdown: true,
+      shutdown: options ?? true,
       expectedState: {
         activeTargets: {},
         activeLimboDocs: [],

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

@@ -296,11 +296,15 @@ abstract class TestRunner {
   }
 
   async shutdown(): Promise<void> {
-    await this.queue.enqueueAndInitiateShutdown(async () => {
+    this.queue.initiateShutdown();
+    const deferred = new Deferred();
+    this.queue.enqueueAndForgetEvenAfterShutdown(async () => {
       if (this.started) {
         await this.doShutdown();
       }
+      deferred.resolve();
     });
+    return deferred.promise;
   }
 
   /** Runs a single SpecStep on this runner. */
@@ -363,7 +367,9 @@ abstract class TestRunner {
     } else if ('restart' in step) {
       return this.doRestart();
     } else if ('shutdown' in step) {
-      return this.doShutdown();
+      return typeof step.shutdown === 'object'
+        ? this.doShutdown(step.shutdown)
+        : this.doShutdown();
     } else if ('applyClientState' in step) {
       // PORTING NOTE: Only used by web multi-tab tests.
       return this.doApplyClientState(step.applyClientState!);
@@ -707,14 +713,20 @@ abstract class TestRunner {
     await this.remoteStore.enableNetwork();
   }
 
-  private async doShutdown(): Promise<void> {
-    await this.remoteStore.shutdown();
-    await this.sharedClientState.shutdown();
-    // We don't delete the persisted data here since multi-clients may still
-    // be accessing it. Instead, we manually remove it at the end of the
-    // test run.
-    await this.persistence.shutdown();
-    this.started = false;
+  private async doShutdown(options?: {expectFailure?: boolean}): Promise<void> {
+    try {
+      await this.remoteStore.shutdown();
+      await this.sharedClientState.shutdown();
+      // We don't delete the persisted data here since multi-clients may still
+      // be accessing it. Instead, we manually remove it at the end of the
+      // test run.
+      await this.persistence.shutdown();
+      expect(options?.expectFailure).to.not.be.true;
+      
+      this.started = false;
+    } catch (e) {
+      expect(options?.expectFailure).to.be.true;
+    }
   }
 
   private async doClearPersistence(): Promise<void> {
@@ -1253,7 +1265,8 @@ export type PersistenceAction =
   | 'Get new document changes'
   | 'Synchronize last document change read time'
   | 'updateClientMetadataAndTryBecomePrimary'
-  | 'getHighestListenSequenceNumber';
+  | 'getHighestListenSequenceNumber'
+  | 'shutdown';
 
 /**
  * Union type for each step. The step consists of exactly one `field`
@@ -1333,7 +1346,7 @@ export interface SpecStep {
   restart?: true;
 
   /** Shut down the client and close it network connection. */
-  shutdown?: true;
+  shutdown?: true | { expectFailure?: boolean };
 
   /**
    * Optional list of expected events.

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

@@ -397,8 +397,8 @@ describe('AsyncQueue', () => {
 
     // After this call, only operations requested via
     // `enqueueAndForgetEvenAfterShutdown` gets executed.
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    queue.enqueueAndInitiateShutdown(() => doStep(2));
+    queue.initiateShutdown();
+    queue.enqueueAndForgetEvenAfterShutdown(() => doStep(2));
     queue.enqueueAndForget(() => doStep(3));
     queue.enqueueAndForgetEvenAfterShutdown(() => doStep(4));