Offline get() improvements.

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.