diff options
author | Michael Lehenbauer <mikelehen@gmail.com> | 2018-03-26 13:24:32 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-03-26 13:24:32 -0700 |
commit | e0e6625f3ef573ebcf10ce6b298939ccbea25532 (patch) | |
tree | 2ed51208ddd994c3fcd581f7b167f9cfd3b27d74 /Firestore | |
parent | f77ec68a8862bd03b430deff48022ffb179172b0 (diff) |
Fix issue that could cause offline get()s to wait up to 10 seconds. (#978)
Port of https://github.com/firebase/firebase-js-sdk/pull/592
FSTOnlineStateTracker was reverting to OnlineState Unknown on every stream attempt
rather than remaining Offline once the offline heuristic had been met (i.e. 2
stream failures or 10 seconds). This means that getDocument() requests made while
offline could be delayed up to 10 seconds each time (or until the next backoff
attempt failed).
Diffstat (limited to 'Firestore')
-rw-r--r-- | Firestore/CHANGELOG.md | 3 | ||||
-rw-r--r-- | Firestore/Example/Tests/SpecTests/json/offline_spec_test.json | 193 | ||||
-rw-r--r-- | Firestore/Source/Remote/FSTOnlineStateTracker.h | 5 | ||||
-rw-r--r-- | Firestore/Source/Remote/FSTOnlineStateTracker.mm | 22 |
4 files changed, 213 insertions, 10 deletions
diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index 50958db..bcd92d0 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,4 +1,7 @@ # Unreleased +- [fixed] Fixed a regression in the Firebase iOS SDK release 4.11.0 that could + cause getDocument() requests made while offline to be delayed by up to 10 + seconds (rather than returning from cache immediately). # v0.10.4 - [changed] If the SDK's attempt to connect to the Cloud Firestore backend diff --git a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json index db8324b..1af4c16 100644 --- a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json @@ -888,5 +888,198 @@ ] } ] + }, + "New queries return immediately with fromCache=true when offline due to stream failures.": { + "describeName": "Offline:", + "itName": "New queries return immediately with fromCache=true when offline due to stream failures.", + "tags": [], + "config": { + "useGarbageCollection": true + }, + "steps": [ + { + "userListen": [ + 2, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + }, + "runBackoffTimer": true + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + }, + "runBackoffTimer": true + }, + "expect": [ + { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + }, + { + "userListen": [ + 4, + { + "path": "collection2", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + }, + "4": { + "query": { + "path": "collection2", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + }, + "expect": [ + { + "query": { + "path": "collection2", + "filters": [], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + } + ] + }, + "New queries return immediately with fromCache=true when offline due to OnlineState timeout.": { + "describeName": "Offline:", + "itName": "New queries return immediately with fromCache=true when offline due to OnlineState timeout.", + "tags": [], + "config": { + "useGarbageCollection": true + }, + "steps": [ + { + "userListen": [ + 2, + { + "path": "collection", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "runTimer": "online_state_timeout", + "expect": [ + { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + }, + { + "userListen": [ + 4, + { + "path": "collection2", + "filters": [], + "orderBys": [] + } + ], + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + }, + "4": { + "query": { + "path": "collection2", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + }, + "expect": [ + { + "query": { + "path": "collection2", + "filters": [], + "orderBys": [] + }, + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false + } + ] + } + ] } } diff --git a/Firestore/Source/Remote/FSTOnlineStateTracker.h b/Firestore/Source/Remote/FSTOnlineStateTracker.h index a414a18..2521c84 100644 --- a/Firestore/Source/Remote/FSTOnlineStateTracker.h +++ b/Firestore/Source/Remote/FSTOnlineStateTracker.h @@ -43,9 +43,10 @@ NS_ASSUME_NONNULL_BEGIN @property(nonatomic, weak) id<FSTOnlineStateDelegate> onlineStateDelegate; /** - * Called by FSTRemoteStore when a watch stream is started. + * Called by FSTRemoteStore when a watch stream is started (including on each backoff attempt). * - * It sets the FSTOnlineState to Unknown and starts the onlineStateTimer if necessary. + * If this is the first attempt, it sets the FSTOnlineState to Unknown and starts the + * onlineStateTimer. */ - (void)handleWatchStreamStart; diff --git a/Firestore/Source/Remote/FSTOnlineStateTracker.mm b/Firestore/Source/Remote/FSTOnlineStateTracker.mm index 41650cd..e782397 100644 --- a/Firestore/Source/Remote/FSTOnlineStateTracker.mm +++ b/Firestore/Source/Remote/FSTOnlineStateTracker.mm @@ -47,7 +47,7 @@ static const NSTimeInterval kOnlineStateTimeout = 10; * Unknown to Offline without waiting for the stream to actually fail (kMaxWatchStreamFailures * times). */ -@property(nonatomic, strong, nullable) FSTDelayedCallback *watchStreamTimer; +@property(nonatomic, strong, nullable) FSTDelayedCallback *onlineStateTimer; /** * Whether the client should log a warning message if it fails to connect to the backend @@ -71,14 +71,15 @@ static const NSTimeInterval kOnlineStateTimeout = 10; } - (void)handleWatchStreamStart { - [self setAndBroadcastState:FSTOnlineStateUnknown]; + if (self.watchStreamFailures == 0) { + [self setAndBroadcastState:FSTOnlineStateUnknown]; - if (self.watchStreamTimer == nil) { - self.watchStreamTimer = [self.queue + FSTAssert(!self.onlineStateTimer, @"onlineStateTimer shouldn't be started yet"); + self.onlineStateTimer = [self.queue dispatchAfterDelay:kOnlineStateTimeout timerID:FSTTimerIDOnlineStateTimeout block:^{ - self.watchStreamTimer = nil; + self.onlineStateTimer = nil; FSTAssert( self.state == FSTOnlineStateUnknown, @"Timer should be canceled if we transitioned to a different state."); @@ -100,6 +101,11 @@ static const NSTimeInterval kOnlineStateTimeout = 10; - (void)handleWatchStreamFailure { if (self.state == FSTOnlineStateOnline) { [self setAndBroadcastState:FSTOnlineStateUnknown]; + + // To get to FSTOnlineStateOnline, updateState: must have been called which would have reset + // our heuristics. + FSTAssert(self.watchStreamFailures == 0, @"watchStreamFailures must be 0"); + FSTAssert(!self.onlineStateTimer, @"onlineStateTimer must be nil"); } else { self.watchStreamFailures++; if (self.watchStreamFailures >= kMaxWatchStreamFailures) { @@ -138,9 +144,9 @@ static const NSTimeInterval kOnlineStateTimeout = 10; } - (void)clearOnlineStateTimer { - if (self.watchStreamTimer) { - [self.watchStreamTimer cancel]; - self.watchStreamTimer = nil; + if (self.onlineStateTimer) { + [self.onlineStateTimer cancel]; + self.onlineStateTimer = nil; } } |