Skip to content

Fix issue that could cause offline get()s to wait up to 10 seconds. #978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 26, 2018
Merged

Fix issue that could cause offline get()s to wait up to 10 seconds. #978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Firestore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
193 changes: 193 additions & 0 deletions Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
]
}
]
}
}
5 changes: 3 additions & 2 deletions Firestore/Source/Remote/FSTOnlineStateTracker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
22 changes: 14 additions & 8 deletions Firestore/Source/Remote/FSTOnlineStateTracker.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ @interface FSTOnlineStateTracker ()
* 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
Expand All @@ -71,14 +71,15 @@ - (instancetype)initWithWorkerDispatchQueue:(FSTDispatchQueue *)queue {
}

- (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.");
Expand All @@ -100,6 +101,11 @@ - (void)handleWatchStreamStart {
- (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) {
Expand Down Expand Up @@ -138,9 +144,9 @@ - (void)logClientOfflineWarningIfNecessary {
}

- (void)clearOnlineStateTimer {
if (self.watchStreamTimer) {
[self.watchStreamTimer cancel];
self.watchStreamTimer = nil;
if (self.onlineStateTimer) {
[self.onlineStateTimer cancel];
self.onlineStateTimer = nil;
}
}

Expand Down