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.