Offline get() improvements.

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';
 
@@ -29,6 +32,8 @@ import {
 } from '../util/helpers';
 import { query } from '../../util/api_helpers';
 
+chai.use(chaiAsPromised);
+
 const Timestamp = firebase.firestore!.Timestamp;
 const FieldValue = firebase.firestore!.FieldValue;
 
@@ -857,26 +862,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;
@@ -604,7 +606,7 @@ abstract class TestRunner {
       );
     }
 
-    if (this.isPrimaryClient) {
+    if (this.isPrimaryClient && this.networkEnabled) {
       // Open should always have happened after a listen
       await this.connection.waitForWatchOpen();
     }
@@ -848,6 +850,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();
@@ -859,6 +862,7 @@ abstract class TestRunner {
   }
 
   private async doEnableNetwork(): Promise<void> {
+    this.networkEnabled = true;
     await this.syncEngine.enableNetwork();
   }
 
@@ -1017,7 +1021,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;