Skip to content

Commit e0e6625

Browse files
mikelehenmikelehen
authored
Fix issue that could cause offline get()s to wait up to 10 seconds. (#978)
Port of firebase/firebase-js-sdk#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).
1 parent f77ec68 commit e0e6625

File tree

4 files changed

+213
-10
lines changed

4 files changed

+213
-10
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
# Unreleased
2+
- [fixed] Fixed a regression in the Firebase iOS SDK release 4.11.0 that could
3+
cause getDocument() requests made while offline to be delayed by up to 10
4+
seconds (rather than returning from cache immediately).
25

36
# v0.10.4
47
- [changed] If the SDK's attempt to connect to the Cloud Firestore backend

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

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -888,5 +888,198 @@
888888
]
889889
}
890890
]
891+
},
892+
"New queries return immediately with fromCache=true when offline due to stream failures.": {
893+
"describeName": "Offline:",
894+
"itName": "New queries return immediately with fromCache=true when offline due to stream failures.",
895+
"tags": [],
896+
"config": {
897+
"useGarbageCollection": true
898+
},
899+
"steps": [
900+
{
901+
"userListen": [
902+
2,
903+
{
904+
"path": "collection",
905+
"filters": [],
906+
"orderBys": []
907+
}
908+
],
909+
"stateExpect": {
910+
"activeTargets": {
911+
"2": {
912+
"query": {
913+
"path": "collection",
914+
"filters": [],
915+
"orderBys": []
916+
},
917+
"resumeToken": ""
918+
}
919+
}
920+
}
921+
},
922+
{
923+
"watchStreamClose": {
924+
"error": {
925+
"code": 14,
926+
"message": "Simulated Backend Error"
927+
},
928+
"runBackoffTimer": true
929+
}
930+
},
931+
{
932+
"watchStreamClose": {
933+
"error": {
934+
"code": 14,
935+
"message": "Simulated Backend Error"
936+
},
937+
"runBackoffTimer": true
938+
},
939+
"expect": [
940+
{
941+
"query": {
942+
"path": "collection",
943+
"filters": [],
944+
"orderBys": []
945+
},
946+
"errorCode": 0,
947+
"fromCache": true,
948+
"hasPendingWrites": false
949+
}
950+
]
951+
},
952+
{
953+
"userListen": [
954+
4,
955+
{
956+
"path": "collection2",
957+
"filters": [],
958+
"orderBys": []
959+
}
960+
],
961+
"stateExpect": {
962+
"activeTargets": {
963+
"2": {
964+
"query": {
965+
"path": "collection",
966+
"filters": [],
967+
"orderBys": []
968+
},
969+
"resumeToken": ""
970+
},
971+
"4": {
972+
"query": {
973+
"path": "collection2",
974+
"filters": [],
975+
"orderBys": []
976+
},
977+
"resumeToken": ""
978+
}
979+
}
980+
},
981+
"expect": [
982+
{
983+
"query": {
984+
"path": "collection2",
985+
"filters": [],
986+
"orderBys": []
987+
},
988+
"errorCode": 0,
989+
"fromCache": true,
990+
"hasPendingWrites": false
991+
}
992+
]
993+
}
994+
]
995+
},
996+
"New queries return immediately with fromCache=true when offline due to OnlineState timeout.": {
997+
"describeName": "Offline:",
998+
"itName": "New queries return immediately with fromCache=true when offline due to OnlineState timeout.",
999+
"tags": [],
1000+
"config": {
1001+
"useGarbageCollection": true
1002+
},
1003+
"steps": [
1004+
{
1005+
"userListen": [
1006+
2,
1007+
{
1008+
"path": "collection",
1009+
"filters": [],
1010+
"orderBys": []
1011+
}
1012+
],
1013+
"stateExpect": {
1014+
"activeTargets": {
1015+
"2": {
1016+
"query": {
1017+
"path": "collection",
1018+
"filters": [],
1019+
"orderBys": []
1020+
},
1021+
"resumeToken": ""
1022+
}
1023+
}
1024+
}
1025+
},
1026+
{
1027+
"runTimer": "online_state_timeout",
1028+
"expect": [
1029+
{
1030+
"query": {
1031+
"path": "collection",
1032+
"filters": [],
1033+
"orderBys": []
1034+
},
1035+
"errorCode": 0,
1036+
"fromCache": true,
1037+
"hasPendingWrites": false
1038+
}
1039+
]
1040+
},
1041+
{
1042+
"userListen": [
1043+
4,
1044+
{
1045+
"path": "collection2",
1046+
"filters": [],
1047+
"orderBys": []
1048+
}
1049+
],
1050+
"stateExpect": {
1051+
"activeTargets": {
1052+
"2": {
1053+
"query": {
1054+
"path": "collection",
1055+
"filters": [],
1056+
"orderBys": []
1057+
},
1058+
"resumeToken": ""
1059+
},
1060+
"4": {
1061+
"query": {
1062+
"path": "collection2",
1063+
"filters": [],
1064+
"orderBys": []
1065+
},
1066+
"resumeToken": ""
1067+
}
1068+
}
1069+
},
1070+
"expect": [
1071+
{
1072+
"query": {
1073+
"path": "collection2",
1074+
"filters": [],
1075+
"orderBys": []
1076+
},
1077+
"errorCode": 0,
1078+
"fromCache": true,
1079+
"hasPendingWrites": false
1080+
}
1081+
]
1082+
}
1083+
]
8911084
}
8921085
}

‎Firestore/Source/Remote/FSTOnlineStateTracker.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,10 @@ NS_ASSUME_NONNULL_BEGIN
4343
@property(nonatomic, weak) id<FSTOnlineStateDelegate> onlineStateDelegate;
4444

4545
/**
46-
* Called by FSTRemoteStore when a watch stream is started.
46+
* Called by FSTRemoteStore when a watch stream is started (including on each backoff attempt).
4747
*
48-
* It sets the FSTOnlineState to Unknown and starts the onlineStateTimer if necessary.
48+
* If this is the first attempt, it sets the FSTOnlineState to Unknown and starts the
49+
* onlineStateTimer.
4950
*/
5051
- (void)handleWatchStreamStart;
5152

‎Firestore/Source/Remote/FSTOnlineStateTracker.mm

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ @interface FSTOnlineStateTracker ()
4747
* Unknown to Offline without waiting for the stream to actually fail (kMaxWatchStreamFailures
4848
* times).
4949
*/
50-
@property(nonatomic, strong, nullable) FSTDelayedCallback *watchStreamTimer;
50+
@property(nonatomic, strong, nullable) FSTDelayedCallback *onlineStateTimer;
5151

5252
/**
5353
* Whether the client should log a warning message if it fails to connect to the backend
@@ -71,14 +71,15 @@ - (instancetype)initWithWorkerDispatchQueue:(FSTDispatchQueue *)queue {
7171
}
7272

7373
- (void)handleWatchStreamStart {
74-
[self setAndBroadcastState:FSTOnlineStateUnknown];
74+
if (self.watchStreamFailures == 0) {
75+
[self setAndBroadcastState:FSTOnlineStateUnknown];
7576

76-
if (self.watchStreamTimer == nil) {
77-
self.watchStreamTimer = [self.queue
77+
FSTAssert(!self.onlineStateTimer, @"onlineStateTimer shouldn't be started yet");
78+
self.onlineStateTimer = [self.queue
7879
dispatchAfterDelay:kOnlineStateTimeout
7980
timerID:FSTTimerIDOnlineStateTimeout
8081
block:^{
81-
self.watchStreamTimer = nil;
82+
self.onlineStateTimer = nil;
8283
FSTAssert(
8384
self.state == FSTOnlineStateUnknown,
8485
@"Timer should be canceled if we transitioned to a different state.");
@@ -100,6 +101,11 @@ - (void)handleWatchStreamStart {
100101
- (void)handleWatchStreamFailure {
101102
if (self.state == FSTOnlineStateOnline) {
102103
[self setAndBroadcastState:FSTOnlineStateUnknown];
104+
105+
// To get to FSTOnlineStateOnline, updateState: must have been called which would have reset
106+
// our heuristics.
107+
FSTAssert(self.watchStreamFailures == 0, @"watchStreamFailures must be 0");
108+
FSTAssert(!self.onlineStateTimer, @"onlineStateTimer must be nil");
103109
} else {
104110
self.watchStreamFailures++;
105111
if (self.watchStreamFailures >= kMaxWatchStreamFailures) {
@@ -138,9 +144,9 @@ - (void)logClientOfflineWarningIfNecessary {
138144
}
139145

140146
- (void)clearOnlineStateTimer {
141-
if (self.watchStreamTimer) {
142-
[self.watchStreamTimer cancel];
143-
self.watchStreamTimer = nil;
147+
if (self.onlineStateTimer) {
148+
[self.onlineStateTimer cancel];
149+
self.onlineStateTimer = nil;
144150
}
145151
}
146152

0 commit comments

Comments
 (0)