Skip to content

Commit a3ab174

Browse files
author
Michael Lehenbauer
committed
Offline get() improvements.
[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.
1 parent de37b46 commit a3ab174

File tree

7 files changed

+153
-31
lines changed

7 files changed

+153
-31
lines changed

Firestore/CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@
55
take effect until you did a full sign-out and sign-in. (#1499)
66
- [changed] Improved how Firestore handles idle queries to reduce the cost of
77
re-listening within 30 minutes.
8+
- [fixed] Fixed an issue where the first `get()` call made after being offline
9+
could incorrectly return cached data without attempting to reach the backend.
10+
- [changed] Changed `get()` to only make 1 attempt to reach the backend before
11+
returning cached data, potentially reducing delays while offline. Previously
12+
it would make 2 attempts, to work around a backend bug.
813

914
# v0.13.1
1015
- [fixed] Fixed an issue where `get(source:.Cache)` could throw an

Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1099,18 +1099,26 @@ - (void)testCanQueueWritesWhileOffline {
10991099
[self awaitExpectations];
11001100
}
11011101

1102-
- (void)testCantGetDocumentsWhileOffline {
1102+
- (void)testCanGetDocumentsWhileOffline {
11031103
FIRDocumentReference *doc = [self documentRef];
11041104
FIRFirestore *firestore = doc.firestore;
11051105
NSDictionary<NSString *, id> *data = @{@"a" : @"b"};
11061106

1107+
XCTestExpectation *failExpectation =
1108+
[self expectationWithDescription:@"offline read with no cached data"];
11071109
XCTestExpectation *onlineExpectation = [self expectationWithDescription:@"online read"];
11081110
XCTestExpectation *networkExpectation = [self expectationWithDescription:@"network online"];
11091111

11101112
__weak FIRDocumentReference *weakDoc = doc;
11111113

11121114
[firestore disableNetworkWithCompletion:^(NSError *error) {
11131115
XCTAssertNil(error);
1116+
1117+
[doc getDocumentWithCompletion:^(FIRDocumentSnapshot *snapshot, NSError *error) {
1118+
XCTAssertNotNil(error);
1119+
[failExpectation fulfill];
1120+
}];
1121+
11141122
[doc setData:data
11151123
completion:^(NSError *_Nullable error) {
11161124
XCTAssertNil(error);

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ @interface FSTSpecTests ()
9191

9292
@implementation FSTSpecTests {
9393
BOOL _gcEnabled;
94+
BOOL _networkEnabled;
9495
}
9596

9697
- (id<FSTPersistence>)persistenceWithGCEnabled:(BOOL)GCEnabled {
@@ -390,10 +391,12 @@ - (void)doRunTimer:(NSString *)timer {
390391
}
391392

392393
- (void)doDisableNetwork {
394+
_networkEnabled = NO;
393395
[self.driver disableNetwork];
394396
}
395397

396398
- (void)doEnableNetwork {
399+
_networkEnabled = YES;
397400
[self.driver enableNetwork];
398401
}
399402

@@ -642,6 +645,10 @@ - (void)validateLimboDocuments {
642645
}
643646

644647
- (void)validateActiveTargets {
648+
if (!_networkEnabled) {
649+
return;
650+
}
651+
645652
// Create a copy so we can modify it in tests
646653
NSMutableDictionary<FSTBoxedTargetID *, FSTQueryData *> *actualTargets =
647654
[NSMutableDictionary dictionaryWithDictionary:self.driver.activeTargets];

Firestore/Example/Tests/SpecTests/json/offline_spec_test.json

Lines changed: 118 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,7 @@
22
"Empty queries are resolved if client goes offline": {
33
"describeName": "Offline:",
44
"itName": "Empty queries are resolved if client goes offline",
5-
"tags": [
6-
"no-android",
7-
"no-ios"
8-
],
5+
"tags": [],
96
"config": {
107
"useGarbageCollection": true,
118
"numClients": 1
@@ -77,10 +74,7 @@
7774
"A successful message delays offline status": {
7875
"describeName": "Offline:",
7976
"itName": "A successful message delays offline status",
80-
"tags": [
81-
"no-android",
82-
"no-ios"
83-
],
77+
"tags": [],
8478
"config": {
8579
"useGarbageCollection": true,
8680
"numClients": 1
@@ -167,9 +161,7 @@
167161
"describeName": "Offline:",
168162
"itName": "Removing all listeners delays \"Offline\" status on next listen",
169163
"tags": [
170-
"eager-gc",
171-
"no-android",
172-
"no-ios"
164+
"eager-gc"
173165
],
174166
"comment": "Marked as no-lru because when a listen is re-added, it gets a new target id rather than reusing one",
175167
"config": {
@@ -290,10 +282,7 @@
290282
"Queries revert to fromCache=true when offline.": {
291283
"describeName": "Offline:",
292284
"itName": "Queries revert to fromCache=true when offline.",
293-
"tags": [
294-
"no-android",
295-
"no-ios"
296-
],
285+
"tags": [],
297286
"config": {
298287
"useGarbageCollection": true,
299288
"numClients": 1
@@ -461,10 +450,7 @@
461450
"Queries with limbo documents handle going offline.": {
462451
"describeName": "Offline:",
463452
"itName": "Queries with limbo documents handle going offline.",
464-
"tags": [
465-
"no-android",
466-
"no-ios"
467-
],
453+
"tags": [],
468454
"config": {
469455
"useGarbageCollection": true,
470456
"numClients": 1
@@ -889,10 +875,7 @@
889875
"New queries return immediately with fromCache=true when offline due to stream failures.": {
890876
"describeName": "Offline:",
891877
"itName": "New queries return immediately with fromCache=true when offline due to stream failures.",
892-
"tags": [
893-
"no-android",
894-
"no-ios"
895-
],
878+
"tags": [],
896879
"config": {
897880
"useGarbageCollection": true,
898881
"numClients": 1
@@ -1074,5 +1057,117 @@
10741057
]
10751058
}
10761059
]
1060+
},
1061+
"Queries return from cache when network disabled": {
1062+
"describeName": "Offline:",
1063+
"itName": "Queries return from cache when network disabled",
1064+
"tags": [],
1065+
"config": {
1066+
"useGarbageCollection": true,
1067+
"numClients": 1
1068+
},
1069+
"steps": [
1070+
{
1071+
"enableNetwork": false,
1072+
"stateExpect": {
1073+
"activeTargets": {},
1074+
"limboDocs": []
1075+
}
1076+
},
1077+
{
1078+
"userListen": [
1079+
2,
1080+
{
1081+
"path": "collection",
1082+
"filters": [],
1083+
"orderBys": []
1084+
}
1085+
],
1086+
"stateExpect": {
1087+
"activeTargets": {
1088+
"2": {
1089+
"query": {
1090+
"path": "collection",
1091+
"filters": [],
1092+
"orderBys": []
1093+
},
1094+
"resumeToken": ""
1095+
}
1096+
}
1097+
},
1098+
"expect": [
1099+
{
1100+
"query": {
1101+
"path": "collection",
1102+
"filters": [],
1103+
"orderBys": []
1104+
},
1105+
"errorCode": 0,
1106+
"fromCache": true,
1107+
"hasPendingWrites": false
1108+
}
1109+
]
1110+
},
1111+
{
1112+
"userUnlisten": [
1113+
2,
1114+
{
1115+
"path": "collection",
1116+
"filters": [],
1117+
"orderBys": []
1118+
}
1119+
],
1120+
"stateExpect": {
1121+
"activeTargets": {}
1122+
}
1123+
},
1124+
{
1125+
"userListen": [
1126+
4,
1127+
{
1128+
"path": "collection",
1129+
"filters": [],
1130+
"orderBys": []
1131+
}
1132+
],
1133+
"stateExpect": {
1134+
"activeTargets": {
1135+
"4": {
1136+
"query": {
1137+
"path": "collection",
1138+
"filters": [],
1139+
"orderBys": []
1140+
},
1141+
"resumeToken": ""
1142+
}
1143+
}
1144+
},
1145+
"expect": [
1146+
{
1147+
"query": {
1148+
"path": "collection",
1149+
"filters": [],
1150+
"orderBys": []
1151+
},
1152+
"errorCode": 0,
1153+
"fromCache": true,
1154+
"hasPendingWrites": false
1155+
}
1156+
]
1157+
},
1158+
{
1159+
"userUnlisten": [
1160+
4,
1161+
{
1162+
"path": "collection",
1163+
"filters": [],
1164+
"orderBys": []
1165+
}
1166+
],
1167+
"stateExpect": {
1168+
"activeTargets": {}
1169+
}
1170+
}
1171+
]
10771172
}
10781173
}

Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -482,10 +482,7 @@
482482
"Cleans up watch state correctly": {
483483
"describeName": "Remote store:",
484484
"itName": "Cleans up watch state correctly",
485-
"tags": [
486-
"no-android",
487-
"no-ios"
488-
],
485+
"tags": [],
489486
"config": {
490487
"useGarbageCollection": false,
491488
"numClients": 1

Firestore/Source/Remote/FSTOnlineStateTracker.mm

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@
2727

2828
// To deal with transient failures, we allow multiple stream attempts before giving up and
2929
// transitioning from OnlineState Unknown to Offline.
30-
static const int kMaxWatchStreamFailures = 2;
30+
// TODO(mikelehen): This used to be set to 2 as a mitigation for b/66228394. @jdimond thinks that
31+
// bug is sufficiently fixed so that we can set this back to 1. If that works okay, we could
32+
// potentially remove this logic entirely.
33+
static const int kMaxWatchStreamFailures = 1;
3134

3235
// To deal with stream attempts that don't succeed or fail in a timely manner, we have a
3336
// timeout for OnlineState to reach Online or Offline. If the timeout is reached, we transition

Firestore/Source/Remote/FSTRemoteStore.mm

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -252,13 +252,20 @@ - (void)sendWatchRequestWithQueryData:(FSTQueryData *)queryData {
252252
- (void)stopListeningToTargetID:(TargetId)targetID {
253253
FSTBoxedTargetID *targetKey = @(targetID);
254254
FSTQueryData *queryData = self.listenTargets[targetKey];
255-
HARD_ASSERT(queryData, "unlistenToTarget: target not currently watched: %s", targetKey);
255+
HARD_ASSERT(queryData, "stopListeningToTargetID: target not currently watched: %s", targetKey);
256256

257257
[self.listenTargets removeObjectForKey:targetKey];
258258
if ([self isNetworkEnabled] && [self.watchStream isOpen]) {
259259
[self sendUnwatchRequestForTargetID:targetKey];
260-
if ([self.listenTargets count] == 0) {
260+
}
261+
if ([self.listenTargets count] == 0) {
262+
if ([self isNetworkEnabled] && [self.watchStream isOpen]) {
261263
[self.watchStream markIdle];
264+
} else if (self.isNetworkEnabled) {
265+
// Revert to OnlineState::Unknown if the watch stream is not open and we have no listeners,
266+
// since without any listens to send we cannot confirm if the stream is healthy and upgrade
267+
// to OnlineState::Online.
268+
[self.onlineStateTracker updateState:OnlineState::Unknown];
262269
}
263270
}
264271
}

0 commit comments

Comments
 (0)