Offline get() improvements. (#1197)

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,11 @@
-# Unreleased
+# 0.7.3 (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. Previously
+  it would make 2 attempts, to work around a backend bug.
+
+# 0.7.2
 - [fixed] Fixed a regression that prevented use of Firestore on ReactNative's
   Expo platform (#1138).
 

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 if (this.canUseNetwork()) {
+        // 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);
       }
     }
   }

packages/firestore/test/integration/api/database.test.ts

@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+import * as chai from 'chai';
+import * as chaiAsPromised from 'chai-as-promised';
+
 import { expect } from 'chai';
 import * as firestore from '@firebase/firestore-types';
 
@@ -32,6 +35,8 @@ import { query } from '../../util/api_helpers';
 import { fail } from '../../../src/util/assert';
 import { Code } from '../../../src/util/error';
 
+chai.use(chaiAsPromised);
+
 const Timestamp = firebase.firestore!.Timestamp;
 const FieldValue = firebase.firestore!.FieldValue;
 
@@ -884,26 +889,25 @@ apiDescribe('Database', persistence => {
     });
   });
 
-  it('can get documents while offline', () => {
-    return withTestDoc(persistence, docRef => {
+  it('can get documents while offline', async () => {
+    await withTestDoc(persistence, async docRef => {
       const firestore = docRef.firestore;
 
-      return firestore.disableNetwork().then(() => {
-        const writePromise = docRef.set({ foo: 'bar' });
-
-        return docRef
-          .get()
-          .then(doc => {
-            expect(doc.metadata.fromCache).to.be.true;
-            return firestore.enableNetwork();
-          })
-          .then(() => writePromise)
-          .then(() => docRef.get())
-          .then(doc => {
-            expect(doc.metadata.fromCache).to.be.false;
-            expect(doc.data()).to.deep.equal({ foo: 'bar' });
-          });
-      });
+      await firestore.disableNetwork();
+      await expect(docRef.get()).to.eventually.be.rejectedWith(
+        'Failed to get document because the client is offline.'
+      );
+
+      const writePromise = docRef.set({ foo: 'bar' });
+      const doc = await docRef.get();
+      expect(doc.metadata.fromCache).to.be.true;
+
+      await firestore.enableNetwork();
+      await writePromise;
+
+      const doc2 = await docRef.get();
+      expect(doc2.metadata.fromCache).to.be.false;
+      expect(doc2.data()).to.deep.equal({ foo: 'bar' });
     });
   });
 

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.
@@ -223,4 +215,26 @@ describeSpec('Offline:', [], () => {
       );
     }
   );
+
+  // TODO(b/114055812): This shouldn't really need to be marked eager-gc
+  specTest(
+    'Queries return from cache when network disabled',
+    ['eager-gc'],
+    () => {
+      const query = Query.atPath(path('collection'));
+      return (
+        spec()
+          .disableNetwork()
+          .userListens(query)
+          .expectEvents(query, { fromCache: true })
+          .userUnlistens(query)
+
+          // There was once a bug where removing the last listener accidentally
+          // reverted us to OnlineState.Unknown, so make sure it works a second time
+          .userListens(query)
+          .expectEvents(query, { fromCache: true })
+          .userUnlistens(query)
+      );
+    }
+  );
 });

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

@@ -77,7 +77,8 @@ describeSpec('Remote store:', [], () => {
         // Close before we get an ack, this should reset our pending
         // target counts.
         .watchStreamCloses(Code.UNAVAILABLE)
-        // This should work now.
+        .expectEvents(query, { fromCache: true })
+
         .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.

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

@@ -375,6 +375,8 @@ abstract class TestRunner {
     [targetId: number]: { query: SpecQuery; resumeToken: string };
   };
 
+  private networkEnabled = true;
+
   private datastore: Datastore;
   private localStore: LocalStore;
   private remoteStore: RemoteStore;
@@ -603,7 +605,7 @@ abstract class TestRunner {
       );
     }
 
-    if (this.isPrimaryClient) {
+    if (this.isPrimaryClient && this.networkEnabled) {
       // Open should always have happened after a listen
       await this.connection.waitForWatchOpen();
     }
@@ -851,6 +853,7 @@ abstract class TestRunner {
   }
 
   private async doDisableNetwork(): Promise<void> {
+    this.networkEnabled = false;
     // Make sure to execute all writes that are currently queued. This allows us
     // to assert on the total number of requests sent before shutdown.
     await this.remoteStore.fillWritePipeline();
@@ -862,6 +865,7 @@ abstract class TestRunner {
   }
 
   private async doEnableNetwork(): Promise<void> {
+    this.networkEnabled = true;
     await this.syncEngine.enableNetwork();
   }
 
@@ -1020,7 +1024,7 @@ abstract class TestRunner {
   }
 
   private async validateActiveTargets(): Promise<void> {
-    if (!this.isPrimaryClient) {
+    if (!this.isPrimaryClient || !this.networkEnabled) {
       expect(this.connection.activeTargets).to.be.empty;
       return;
     }

packages/rxfire/firestore/collection/index.ts

@@ -75,7 +75,10 @@ function processIndividualChange(
       }
       break;
     case 'modified':
-      if (combined[change.oldIndex] == null || combined[change.oldIndex].doc.id == change.doc.id) {
+      if (
+        combined[change.oldIndex] == null ||
+        combined[change.oldIndex].doc.id == change.doc.id
+      ) {
         // When an item changes position we first remove it
         // and then add it's new position
         if (change.oldIndex !== change.newIndex) {
@@ -87,7 +90,10 @@ function processIndividualChange(
       }
       break;
     case 'removed':
-      if (combined[change.oldIndex] && combined[change.oldIndex].doc.id == change.doc.id) {
+      if (
+        combined[change.oldIndex] &&
+        combined[change.oldIndex].doc.id == change.doc.id
+      ) {
         combined.splice(change.oldIndex, 1);
       }
       break;