Fix issue that could cause offline get()s to wait up to 10 seconds. (#592)

OnlineStateTracker was reverting to OnlineState Unknown on every stream attempt
rather than remaining Offline once the offline heuristic had been met (i.e. 2
stream failures or 10 seconds). This means that get() requests made while
offline could be delayed up to 10 seconds each time (or until the next backoff
attempt failed).

Author: mikelehen

packages/firestore/CHANGELOG.md

@@ -1,4 +1,9 @@
 # Unreleased
+- [fixed] Fixed a regression in the Firebase JS release 4.11.0 that could
+  cause get() requests made while offline to be delayed by up to 10
+  seconds (rather than returning from cache immediately).
+
+# 0.3.6
 - [fixed] Fixed a regression in the Firebase JS release 4.11.0 that could
   cause a crash if a user signs out while the client is offline, resulting in
   an error of "Attempted to schedule multiple operations with timer id

packages/firestore/src/remote/online_state_tracker.ts

@@ -74,15 +74,20 @@ export class OnlineStateTracker {
   ) {}
 
   /**
-   * Called by RemoteStore when a watch stream is started.
+   * Called by RemoteStore when a watch stream is started (including on each
+   * backoff attempt).
    *
-   * It sets the OnlineState to Unknown and starts the onlineStateTimer
-   * if necessary.
+   * If this is the first attempt, it sets the OnlineState to Unknown and starts
+   * the onlineStateTimer.
    */
   handleWatchStreamStart(): void {
-    this.setAndBroadcast(OnlineState.Unknown);
+    if (this.watchStreamFailures === 0) {
+      this.setAndBroadcast(OnlineState.Unknown);
 
-    if (this.onlineStateTimer === null) {
+      assert(
+        this.onlineStateTimer === null,
+        `onlineStateTimer shouldn't be started yet`
+      );
       this.onlineStateTimer = this.asyncQueue.enqueueAfterDelay(
         TimerId.OnlineStateTimeout,
         ONLINE_STATE_TIMEOUT_MS,
@@ -119,6 +124,11 @@ export class OnlineStateTracker {
   handleWatchStreamFailure(): void {
     if (this.state === OnlineState.Online) {
       this.setAndBroadcast(OnlineState.Unknown);
+
+      // To get to OnlineState.Online, set() must have been called which would
+      // have reset our heuristics.
+      assert(this.watchStreamFailures === 0, 'watchStreamFailures must be 0');
+      assert(this.onlineStateTimer === null, 'onlineStateTimer must be null');
     } else {
       this.watchStreamFailures++;
       if (this.watchStreamFailures >= MAX_WATCH_STREAM_FAILURES) {

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

@@ -181,4 +181,47 @@ describeSpec('Offline:', [], () => {
         })
     );
   });
+
+  specTest(
+    'New queries return immediately with fromCache=true when offline due to ' +
+      'stream failures.',
+    [],
+    () => {
+      const query1 = Query.atPath(path('collection'));
+      const query2 = Query.atPath(path('collection2'));
+      return (
+        spec()
+          .userListens(query1)
+          // 2 Failures should mark the client offline and trigger an empty
+          // fromCache event.
+          .watchStreamCloses(Code.UNAVAILABLE)
+          .watchStreamCloses(Code.UNAVAILABLE)
+          .expectEvents(query1, { fromCache: true })
+
+          // A new query should immediately return from cache.
+          .userListens(query2)
+          .expectEvents(query2, { fromCache: true })
+      );
+    }
+  );
+
+  specTest(
+    'New queries return immediately with fromCache=true when offline due to ' +
+      'OnlineState timeout.',
+    [],
+    () => {
+      const query1 = Query.atPath(path('collection'));
+      const query2 = Query.atPath(path('collection2'));
+      return (
+        spec()
+          .userListens(query1)
+          .runTimer(TimerId.OnlineStateTimeout)
+          .expectEvents(query1, { fromCache: true })
+
+          // A new query should immediately return from cache.
+          .userListens(query2)
+          .expectEvents(query2, { fromCache: true })
+      );
+    }
+  );
 });