Skip to content

Commit a47788a

Browse files
mikelehenmikelehen
authored
Offline get() improvements. (firebase#2)
[Port of firebase/firebase-js-sdk#1133] 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 be03713 commit a47788a

File tree

6 files changed

+146
-29
lines changed

6 files changed

+146
-29
lines changed

‎firebase-firestore/CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
11
# Unreleased
2+
- [fixed] Fixed an issue where the first `get()` call made after being offline
3+
could incorrectly return cached data without attempting to reach the backend.
4+
- [changed] Changed `get()` to only make 1 attempt to reach the backend before
5+
returning cached data, potentially reducing delays while offline. Previously
6+
it would make 2 attempts, to work around a backend bug.
7+
8+
# 17.1.0
29
- [feature] Added `FieldValue.arrayUnion()` and `FieldValue.arrayRemove()` to
310
atomically add and remove elements from an array field in a document.
411
- [feature] Added `Query.whereArrayContains()` query operator to find documents

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/OnlineStateTracker.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,10 @@ interface OnlineStateCallback {
4646

4747
// To deal with transient failures, we allow multiple stream attempts before giving up and
4848
// transitioning from OnlineState.UNKNOWN to OFFLINE.
49-
private static final int MAX_WATCH_STREAM_FAILURES = 2;
49+
// TODO(mikelehen): This used to be set to 2 as a mitigation for b/66228394. @jdimond thinks that
50+
// bug is sufficiently fixed so that we can set this back to 1. If that works okay, we could
51+
// potentially remove this logic entirely.
52+
private static final int MAX_WATCH_STREAM_FAILURES = 1;
5053

5154
// To deal with stream attempts that don't succeed or fail in a timely manner, we have a
5255
// timeout for OnlineState to reach ONLINE or OFFLINE. If the timeout is reached, we transition

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,8 +325,16 @@ public void stopListening(int targetId) {
325325
// The watch stream might not be started if we're in a disconnected state
326326
if (watchStream.isOpen()) {
327327
sendUnwatchRequest(targetId);
328-
if (listenTargets.isEmpty()) {
328+
}
329+
330+
if (listenTargets.isEmpty()) {
331+
if (watchStream.isOpen()) {
329332
watchStream.markIdle();
333+
} else if (this.canUseNetwork()) {
334+
// Revert to OnlineState.UNKNOWN if the watch stream is not open and we have no listeners,
335+
// since without any listens to send we cannot confirm if the stream is healthy and upgrade
336+
// to OnlineState.ONLINE.
337+
this.onlineStateTracker.updateState(OnlineState.UNKNOWN);
330338
}
331339
}
332340
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
133133
: Sets.newHashSet("no-android", "benchmark", "multi-client");
134134

135135
private boolean garbageCollectionEnabled;
136+
private boolean networkEnabled = true;
136137

137138
//
138139
// Parts of the Firestore system that the spec tests need to control.
@@ -670,6 +671,7 @@ private void doDrainQueue() throws Exception {
670671
}
671672

672673
private void doDisableNetwork() throws Exception {
674+
networkEnabled = false;
673675
queue.runSync(
674676
() -> {
675677
// Make sure to execute all writes that are currently queued. This allows us
@@ -680,6 +682,7 @@ private void doDisableNetwork() throws Exception {
680682
}
681683

682684
private void doEnableNetwork() throws Exception {
685+
networkEnabled = true;
683686
queue.runSync(() -> remoteStore.enableNetwork());
684687
}
685688

@@ -939,6 +942,10 @@ private void validateLimboDocs() {
939942
}
940943

941944
private void validateActiveTargets() {
945+
if (!networkEnabled) {
946+
return;
947+
}
948+
942949
// Create a copy so we can modify it in tests
943950
Map<Integer, QueryData> actualTargets = new HashMap<>(datastore.activeTargets());
944951

‎firebase-firestore/src/test/resources/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": ["eager-gc"],
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
}

‎firebase-firestore/src/test/resources/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

0 commit comments

Comments
 (0)