Modify Firestore AsyncQueue to be more robust

Firestore was becoming stuck because whatever operation was on `tail` seemed to never complete. Once in this state, all future writes would simply queue up in memory and never process, losing user data.

My theory is that disk errors (or hangs) are transient, and that trying again will probably work. This refactors AsyncQueue to add a few things:
1. Timeouts for all async operations
2. Automatic retry for each operation (up to a limit)
3. Logging when an operation fails

This is pretty hacky because attempting to link `@firebase/firestore` ended up including multiple versions of firebase, and I tweaked rollup to be faster and emit more readable code.

Author: rogueg

packages/firestore/index.ts

@@ -15,7 +15,7 @@
  * limitations under the License.
  */
 
-import firebase from '@firebase/app';
+// import firebase from '@firebase/app';
 import { FirebaseNamespace } from '@firebase/app-types';
 
 import { Firestore } from './src/api/database';
@@ -38,4 +38,5 @@ export function registerFirestore(instance: FirebaseNamespace): void {
   instance.registerVersion(name, version);
 }
 
-registerFirestore(firebase);
+// @ts-ignore
+window.FIRESTORE_REGISTER_FN = registerFirestore;

packages/firestore/rollup.config.js

@@ -102,11 +102,9 @@ export function resolveMemoryExterns(deps, externsId, referencedBy) {
 const es5BuildPlugins = [
   typescriptPlugin({
     typescript,
-    transformers: appendPrivatePrefixTransformers,
     cacheRoot: `./.cache/es5.mangled/`
   }),
-  json(),
-  terser(manglePrivatePropertiesOptions)
+  json()
 ];
 
 const es2017BuildPlugins = [
@@ -132,41 +130,6 @@ const browserBuilds = [
     plugins: es5BuildPlugins,
     external: resolveBrowserExterns
   },
-  // ES5 ESM Build (memory-only)
-  {
-    input: 'index.memory.ts',
-    output: {
-      file: path.resolve('./memory', memoryPkg.module),
-      format: 'es',
-      sourcemap: true
-    },
-    plugins: es5BuildPlugins,
-    external: (id, referencedBy) =>
-      resolveMemoryExterns(browserDeps, id, referencedBy)
-  },
-  // ES2017 ESM build (with persistence)
-  {
-    input: 'index.ts',
-    output: {
-      file: pkg.esm2017,
-      format: 'es',
-      sourcemap: true
-    },
-    plugins: es5BuildPlugins,
-    external: resolveBrowserExterns
-  },
-  // ES2017 ESM build (memory-only)
-  {
-    input: 'index.memory.ts',
-    output: {
-      file: path.resolve('./memory', memoryPkg.esm2017),
-      format: 'es',
-      sourcemap: true
-    },
-    plugins: es2017BuildPlugins,
-    external: (id, referencedBy) =>
-      resolveMemoryExterns(browserDeps, id, referencedBy)
-  },
   // ES5 CJS Build (with persistence)
   //
   // This build is based on the mangling in the ESM build above, since
@@ -175,19 +138,6 @@ const browserBuilds = [
     input: pkg.module,
     output: { file: pkg.browser, format: 'cjs', sourcemap: true },
     plugins: [sourcemaps()]
-  },
-  // ES5 CJS Build (memory-only)
-  //
-  // This build is based on the mangling in the ESM build above, since
-  // Terser's Property name mangling doesn't work well with CJS's syntax.
-  {
-    input: path.resolve('./memory', memoryPkg.module),
-    output: {
-      file: path.resolve('./memory', memoryPkg.browser),
-      format: 'cjs',
-      sourcemap: true
-    },
-    plugins: [sourcemaps()]
   }
 ];
 
@@ -205,27 +155,6 @@ const nodeBuildPlugins = [
 ];
 
 const nodeBuilds = [
-  // ES5 CJS build (with persistence)
-  {
-    input: 'index.node.ts',
-    output: [{ file: pkg.main, format: 'cjs', sourcemap: true }],
-    plugins: nodeBuildPlugins,
-    external: resolveNodeExterns
-  },
-  // ES5 CJS build (memory-only)
-  {
-    input: 'index.node.memory.ts',
-    output: [
-      {
-        file: path.resolve('./memory', memoryPkg.main),
-        format: 'cjs',
-        sourcemap: true
-      }
-    ],
-    plugins: nodeBuildPlugins,
-    external: (id, referencedBy) =>
-      resolveMemoryExterns(nodeDeps, id, referencedBy)
-  }
 ];
 
 export default [...browserBuilds, ...nodeBuilds];

packages/firestore/src/core/firestore_client.ts

@@ -433,11 +433,6 @@ export class FirestoreClient {
     });
   }
 
-  stillWorking(): boolean {
-    const ts = this.asyncQueue.operationStart;
-    return ts === 0 || ts > Date.now() - (1000 * 60);
-  }
-
   /**
    * Returns a Promise that resolves when all writes that were pending at the time this
    * method was called received server acknowledgement. An acknowledgement can be either acceptance

packages/firestore/src/util/async_queue.ts

@@ -191,9 +191,18 @@ class DelayedOperation<T extends unknown> implements CancelablePromise<T> {
   }
 }
 
+interface PendingOp {
+  op: Function
+  signature: string
+  attempts: number
+  resolve: Function
+  reject: Function
+  promise: Promise<void>
+};
+
 export class AsyncQueue {
-  // The last promise in the queue.
-  private tail: Promise<unknown> = Promise.resolve();
+  // all operations waiting to be processed
+  private pending: PendingOp[] = [];
 
   // Is this AsyncQueue being shut down? Once it is set to true, it will not
   // be changed again.
@@ -209,9 +218,7 @@ export class AsyncQueue {
   // Flag set while there's an outstanding AsyncQueue operation, used for
   // assertion sanity-checks.
   private operationInProgress = false;
-
-  operationSignature = '<none>';
-  operationStart = 0;
+  private operationStart = 0;
 
   // List of TimerIds to fast-forward delays for.
   private timerIdsToSkip: TimerId[] = [];
@@ -283,40 +290,85 @@ export class AsyncQueue {
   }
 
   private enqueueInternal<T extends unknown>(op: () => Promise<T>): Promise<T> {
-    const newTail = this.tail.then(() => {
-      this.operationStart = Date.now();
-      this.operationSignature = op.toString();
-      this.operationInProgress = true;
-      return op()
-        .catch((error: FirestoreError) => {
-          this.failure = error;
-          this.operationInProgress = false;
-          const message = error.stack || error.message || '';
-          log.error('INTERNAL UNHANDLED ERROR: ', message);
-
-          // Escape the promise chain and throw the error globally so that
-          // e.g. any global crash reporting library detects and reports it.
-          // (but not for simulated errors in our tests since this breaks mocha)
-          if (message.indexOf('Firestore Test Simulated Error') < 0) {
-            setTimeout(() => {
-              throw error;
-            }, 0);
-          }
-
-          // Re-throw the error so that this.tail becomes a rejected Promise and
-          // all further attempts to chain (via .then) will just short-circuit
-          // and return the rejected Promise.
-          throw error;
-        })
-        .then(result => {
-          this.operationStart = 0;
-          this.operationSignature = '<none>';
-          this.operationInProgress = false;
-          return result;
-        });
+    let resolve: Function, reject: Function;
+    const promise = new Promise((res, rej) => {
+      resolve = res;
+      reject = rej;
+    });
+
+    // @ts-ignore
+    this.pending.push({op, attempts: 0, resolve, reject, promise} as PendingOp);
+
+    if (!this.operationInProgress) {
+      this.runNext();
+    }
+
+    return promise as Promise<T>;
+  }
+
+  private withTimeout<T extends unknown> (promise: Promise<T>, ms: number): Promise<T> {
+    let hasFinished = false;
+
+    const timeout = new Promise((_, reject) => {
+      setTimeout(() => {
+        if (!hasFinished) {
+          reject('Timeout');
+        }
+      }, ms);
+    });
+
+    return Promise.race([promise.then(() => { hasFinished = true; }), timeout]) as Promise<T>;
+  }
+
+  private runNext (): void {
+    if (this.operationInProgress || this.pending.length === 0) { return; }
+
+    const toRun = this.pending[0];
+    toRun.attempts++;
+    this.operationStart = Date.now();
+    this.operationInProgress = true;
+
+    let runningOp = toRun.op();
+
+
+    // @ts-ignore
+    if (window.TRIGGER_FAILED_FIRESTORE_OP) {
+      runningOp = new Promise(() => {});
+      // @ts-ignore
+      window.TRIGGER_FAILED_FIRESTORE_OP = false;
+    }
+
+    this.withTimeout(runningOp, 10 * 1000).then((res: unknown) => {
+      this.operationInProgress = false;
+      this.pending.shift();
+      this.runNext();
+      return res;
+    })
+    .catch((e: FirestoreError) => {
+      this.operationInProgress = false;
+
+      // @ts-ignore
+      window.Profile('log.firestoreError', {message: e.message, signature: toRun.op.toString(), attempt: toRun.attempts, duration: Date.now() - this.operationStart});
+
+      // @ts-ignore
+      window.Bugsnag.notify(e);
+
+      // give up on the operation, throw an error, and continue on
+      if (toRun.attempts >= 3) {
+        this.pending.shift();
+
+        // Escape the promise chain and throw the error globally so that
+        // any global crash reporting library detects and reports it.
+        // (but not for simulated errors in our tests since this breaks mocha)
+        if ((e.message || '').indexOf('Firestore Test Simulated Error') < 0) {
+          setTimeout(() => { throw e; }, 0);
+        }
+      }
+
+      // Add a slight delay before trying again. If it was an I/O problem,
+      // waiting often helps.
+      setTimeout(() => this.runNext(), 5000);
     });
-    this.tail = newTail;
-    return newTail;
   }
 
   /**
@@ -384,11 +436,9 @@ export class AsyncQueue {
     // operations. Keep draining the queue until the tail is no longer advanced,
     // which indicates that no more new operations were enqueued and that all
     // operations were executed.
-    let currentTail: Promise<unknown>;
-    do {
-      currentTail = this.tail;
-      await currentTail;
-    } while (currentTail !== this.tail);
+    while (this.pending.length > 0) {
+      this.runNext();
+    }
   }
 
   /**