From c7e1ec956dac929e034f6a3e9a76d8b3663fc885 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Thu, 16 Aug 2018 16:38:36 -0700 Subject: [PATCH] Offline get() improvements. 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. --- packages/firestore/CHANGELOG.md | 8 +++++++- .../src/remote/online_state_tracker.ts | 5 ++++- packages/firestore/src/remote/remote_store.ts | 11 ++++++++-- .../test/unit/specs/offline_spec.test.ts | 20 ++++++------------- .../test/unit/specs/remote_store_spec.test.ts | 2 ++ 5 files changed, 28 insertions(+), 18 deletions(-) diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index 2a268b804aa..abd9b5608de 100644 --- a/packages/firestore/CHANGELOG.md +++ b/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 diff --git a/packages/firestore/src/remote/online_state_tracker.ts b/packages/firestore/src/remote/online_state_tracker.ts index 4938a0b1f76..41e1422a242 100644 --- a/packages/firestore/src/remote/online_state_tracker.ts +++ b/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. diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index a16435e4932..7167c762c9e 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/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. diff --git a/packages/firestore/test/unit/specs/offline_spec.test.ts b/packages/firestore/test/unit/specs/offline_spec.test.ts index 42f3af45751..0441dddc7a6 100644 --- a/packages/firestore/test/unit/specs/offline_spec.test.ts +++ b/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. diff --git a/packages/firestore/test/unit/specs/remote_store_spec.test.ts b/packages/firestore/test/unit/specs/remote_store_spec.test.ts index ef758d4d171..a9b978a022d 100644 --- a/packages/firestore/test/unit/specs/remote_store_spec.test.ts +++ b/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.