Offline get() improvements. #1197
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
August 17, 2018 10:00
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.
mikelehen
requested review from
gsoltis,
rsgowman and
schmidt-sebastian
as code owners
mikelehen
commented
Sep 5, 2018
| await firestore.disableNetwork(); | ||
| await expect(docRef.get()).to.eventually.be.rejectedWith( | ||
| 'Failed to get document because the client is offline.' | ||
| ); |
There was a problem hiding this comment.
This initial failing get() call existed in the android test and caught the regression, so I've added it to web as well. The rest of the test is unchanged except to use async/await.
wilhuff
approved these changes
Sep 5, 2018
packages/firestore/CHANGELOG.md
Outdated
| - [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. |
There was a problem hiding this comment.
Is it worth noting here that the two attempts mechanism was a work around for a specific backend bug that's no longer in effect?
| @@ -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. | |||
There was a problem hiding this comment.
I don't understand what this comment means anymore now that we've raised the event above.
There was a problem hiding this comment.
Hrm, yeah. TBH I don't know exactly what it was referring to before, so I just removed it.
mikelehen
pushed a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Sep 5, 2018
[Port of firebase/firebase-js-sdk#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.
mikelehen
added a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Sep 6, 2018
[Port of firebase/firebase-js-sdk#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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Roll-forward of c7e1ec9 (which was reviewed, submitted, and then reverted due to regression). The regression fix and added tests are in 3be0938]
@jdimond.
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.