Offline get() improvements. (#1133)

1. Changed MAX_WATCH_STREAM_FAILURES to 1 per conversation with @wilhuff and
   @jdimond.
2. Fixed a "race" where when a get() triggered a watch stream error, we'd
   restart the stream, then get() would remove its listener, and so we had no
   listener left once the watch stream was "opened". Without a listen to send
   we'd be stuck in Offline state (even though we should be Unknown whenever
   we have no listens to send), resulting in the next get() returning data
   from cache without even attempting to reach the backend.

Author: mikelehen

packages/firestore/CHANGELOG.md

@@ -1,4 +1,10 @@
-# 0.7.0 (Unreleased)
+# 0.7.1 (Unreleased)
+- [fixed] Fixed an issue where the first `get()` call made after being offline
+  could incorrectly return cached data without attempting to reach the backend.
+- [changed] Changed `get()` to only make 1 attempt to reach the backend before
+  returning cached data, potentially reducing delays while offline.
+
+# 0.7.0
 - [fixed] Fixed `get({source: 'cache'})` to be able to return nonexistent
   documents from cache.
 - [changed] Prepared the persistence layer to allow shared access from multiple

packages/firestore/src/remote/online_state_tracker.ts

@@ -25,7 +25,10 @@ const LOG_TAG = 'OnlineStateTracker';
 
 // To deal with transient failures, we allow multiple stream attempts before
 // giving up and transitioning from OnlineState.Unknown to Offline.
-const MAX_WATCH_STREAM_FAILURES = 2;
+// TODO(mikelehen): This used to be set to 2 as a mitigation for b/66228394.
+// @jdimond thinks that bug is sufficiently fixed so that we can set this back
+// to 1. If that works okay, we could potentially remove this logic entirely.
+const MAX_WATCH_STREAM_FAILURES = 1;
 
 // To deal with stream attempts that don't succeed or fail in a timely manner,
 // we have a timeout for OnlineState to reach Online or Offline.

packages/firestore/src/remote/remote_store.ts

@@ -243,8 +243,16 @@ export class RemoteStore implements TargetMetadataProvider {
     delete this.listenTargets[targetId];
     if (this.watchStream.isOpen()) {
       this.sendUnwatchRequest(targetId);
-      if (objUtils.isEmpty(this.listenTargets)) {
+    }
+
+    if (objUtils.isEmpty(this.listenTargets)) {
+      if (this.watchStream.isOpen()) {
         this.watchStream.markIdle();
+      } else {
+        // Revert to OnlineState.Unknown if the watch stream is not open and we
+        // have no listeners, since without any listens to send we cannot
+        // confirm if the stream is healthy and upgrade to OnlineState.Online.
+        this.onlineStateTracker.set(OnlineState.Unknown);
       }
     }
   }
@@ -330,7 +338,6 @@ export class RemoteStore implements TargetMetadataProvider {
     // If we still need the watch stream, retry the connection.
     if (this.shouldStartWatchStream()) {
       this.onlineStateTracker.handleWatchStreamFailure(error);
-
       this.startWatchStream();
     } else {
       // No need to restart watch stream because there are no active targets.

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

@@ -28,8 +28,6 @@ describeSpec('Offline:', [], () => {
     return (
       spec()
         .userListens(query)
-        // second error triggers event
-        .watchStreamCloses(Code.UNAVAILABLE)
         .watchStreamCloses(Code.UNAVAILABLE)
         .expectEvents(query, {
           fromCache: true,
@@ -49,8 +47,7 @@ describeSpec('Offline:', [], () => {
         .watchAcks(query)
         // first error triggers unknown state
         .watchStreamCloses(Code.UNAVAILABLE)
-        // getting two more errors triggers offline state
-        .watchStreamCloses(Code.UNAVAILABLE)
+        // second error triggers offline state
         .watchStreamCloses(Code.UNAVAILABLE)
         .expectEvents(query, {
           fromCache: true,
@@ -71,8 +68,7 @@ describeSpec('Offline:', [], () => {
       return (
         spec()
           .userListens(query)
-          // getting two errors triggers offline state
-          .watchStreamCloses(Code.UNAVAILABLE)
+          // error triggers offline state
           .watchStreamCloses(Code.UNAVAILABLE)
           .expectEvents(query, {
             fromCache: true,
@@ -83,11 +79,10 @@ describeSpec('Offline:', [], () => {
           // If the next (already scheduled) connection attempt fails, we'll move
           // to unknown since there are no listeners, and stop trying to connect.
           .watchStreamCloses(Code.UNAVAILABLE)
-          // Suppose sometime later we listen again, it should take two failures
+          // Suppose sometime later we listen again, it should take one failure
           // before we get cached data.
           .userListens(query)
           .watchStreamCloses(Code.UNAVAILABLE)
-          .watchStreamCloses(Code.UNAVAILABLE)
           .expectEvents(query, {
             fromCache: true,
             hasPendingWrites: false
@@ -107,8 +102,7 @@ describeSpec('Offline:', [], () => {
         // first error triggers unknown state
         .watchStreamCloses(Code.UNAVAILABLE)
         .restoreListen(query, 'resume-token-1000')
-        // getting two more errors triggers offline state and fromCache: true
-        .watchStreamCloses(Code.UNAVAILABLE)
+        // second error triggers offline state and fromCache: true
         .watchStreamCloses(Code.UNAVAILABLE)
         .expectEvents(query, { fromCache: true })
         // Going online and getting a CURRENT message triggers fromCache: false
@@ -136,8 +130,7 @@ describeSpec('Offline:', [], () => {
         // first error triggers unknown state
         .watchStreamCloses(Code.UNAVAILABLE)
         .restoreListen(query, 'resume-token-1001')
-        // getting two more errors triggers offline state.
-        .watchStreamCloses(Code.UNAVAILABLE)
+        // second error triggers offline state.
         .watchStreamCloses(Code.UNAVAILABLE)
         .watchAcksFull(query, 1001)
         .watchAcksFull(limboQuery, 1001)
@@ -191,10 +184,9 @@ describeSpec('Offline:', [], () => {
       return (
         spec()
           .userListens(query1)
-          // 2 Failures should mark the client offline and trigger an empty
+          // After failure, we 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.

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

@@ -77,6 +77,7 @@ describeSpec('Remote store:', [], () => {
         // Close before we get an ack, this should reset our pending
         // target counts.
         .watchStreamCloses(Code.UNAVAILABLE)
+        .expectEvents(query, { fromCache: true })
         // This should work now.
         .watchAcksFull(query, 1001, doc1)
         .expectEvents(query, { added: [doc1] })
@@ -97,6 +98,7 @@ describeSpec('Remote store:', [], () => {
           // close the stream (this should trigger retry with backoff; but don't
           // run it in an attempt to reproduce b/74749605).
           .watchStreamCloses(Code.UNAVAILABLE, { runBackoffTimer: false })
+          .expectEvents(query, { fromCache: true })
 
           // Because we didn't let the backoff timer run and restart the watch
           // stream, there will be no active targets.