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).
firebase · Mar 26, 2018 · e0e6625 · e0e6625
1 parent f77ec68
commit e0e6625
Show file tree

Hide file tree

Showing 4 changed files with 213 additions and 10 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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.");
@@ -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) {
@@ -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;
   }
 }