Port of fix for b/67042460 (retry more often before considering client offline) (#403)

This ensures FSTRemoteStore always tries to connect at least twice before surfacing an FSTOnlineStateFailed
event to external code (which may trigger gets to fail, cached data to be surfaced, etc.).

5 files changed, 208 insertions, 97 deletions

diff --git a/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m b/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m
index b4c0b02..b83942b 100644
--- a/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m
+++ b/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.m
@@ -263,8 +263,10 @@ NS_ASSUME_NONNULL_BEGIN
       [NSError errorWithDomain:FIRFirestoreErrorDomain code:errorCode userInfo:userInfo];
 
   [self.datastore failWatchStreamWithError:error];
-  // Unlike web, stream should re-open synchronously
-  FSTAssert(self.datastore.isWatchStreamOpen, @"Watch stream is open");
+  // Unlike web, stream should re-open synchronously (if we have any listeners)
+  if (self.queryListeners.count > 0) {
+    FSTAssert(self.datastore.isWatchStreamOpen, @"Watch stream is open");
+  }
 }
 
 - (NSDictionary<FSTDocumentKey *, FSTBoxedTargetID *> *)currentLimboDocuments {
diff --git a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
index e58bae1..f542a6e 100644
--- a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json
@@ -35,6 +35,14 @@
             "code": 14,
             "message": "Simulated Backend Error"
           }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          }
         },
         "expect": [
           {
@@ -116,6 +124,14 @@
             "code": 14,
             "message": "Simulated Backend Error"
           }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          }
         },
         "expect": [
           {
@@ -147,5 +163,136 @@
         }
       }
     ]
+  },
+  "Removing all listeners delays \"Offline\" status on next listen": {
+    "describeName": "Offline:",
+    "itName": "Removing all listeners delays \"Offline\" status on next listen",
+    "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"
+          }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          }
+        },
+        "expect": [
+          {
+            "query": {
+              "path": "collection",
+              "filters": [],
+              "orderBys": []
+            },
+            "errorCode": 0,
+            "fromCache": true,
+            "hasPendingWrites": false
+          }
+        ]
+      },
+      {
+        "userUnlisten": [
+          2,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {}
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          }
+        }
+      },
+      {
+        "userListen": [
+          4,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {
+            "4": {
+              "query": {
+                "path": "collection",
+                "filters": [],
+                "orderBys": []
+              },
+              "resumeToken": ""
+            }
+          }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          }
+        }
+      },
+      {
+        "watchStreamClose": {
+          "error": {
+            "code": 14,
+            "message": "Simulated Backend Error"
+          }
+        },
+        "expect": [
+          {
+            "query": {
+              "path": "collection",
+              "filters": [],
+              "orderBys": []
+            },
+            "errorCode": 0,
+            "fromCache": true,
+            "hasPendingWrites": false
+          }
+        ]
+      }
+    ]
   }
 }
diff --git a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json
index edb0751..26bb520 100644
--- a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json
@@ -489,19 +489,7 @@
             "code": 14,
             "message": "Simulated Backend Error"
           }
-        },
-        "expect": [
-          {
-            "query": {
-              "path": "collection",
-              "filters": [],
-              "orderBys": []
-            },
-            "errorCode": 0,
-            "fromCache": true,
-            "hasPendingWrites": false
-          }
-        ]
+        }
       },
       {
         "watchAck": [
diff --git a/Firestore/Example/Tests/SpecTests/json/write_spec_test.json b/Firestore/Example/Tests/SpecTests/json/write_spec_test.json
index 60ed107..51d1aee 100644
--- a/Firestore/Example/Tests/SpecTests/json/write_spec_test.json
+++ b/Firestore/Example/Tests/SpecTests/json/write_spec_test.json
@@ -4372,9 +4372,9 @@
       }
     ]
   },
-  "Writes that fail with code cancelled are retried": {
+  "Writes that fail with code resource_exhausted are not rejected": {
     "describeName": "Writes:",
-    "itName": "Writes that fail with code cancelled are retried",
+    "itName": "Writes that fail with code resource_exhausted are not rejected",
     "tags": [],
     "config": {
       "useGarbageCollection": true
@@ -4435,73 +4435,16 @@
       {
         "failWrite": {
           "error": {
-            "code": 1
+            "code": 8
           },
           "expectUserCallback": false
         }
-      },
-      {
-        "writeAck": {
-          "version": 1000,
-          "expectUserCallback": true
-        }
-      },
-      {
-        "watchAck": [
-          2
-        ]
-      },
-      {
-        "watchEntity": {
-          "docs": [
-            [
-              "collection/key",
-              0,
-              {
-                "foo": "bar"
-              }
-            ]
-          ],
-          "targets": [
-            2
-          ]
-        }
-      },
-      {
-        "watchCurrent": [
-          [
-            2
-          ],
-          "resume-token-1000"
-        ],
-        "watchSnapshot": 1000,
-        "expect": [
-          {
-            "query": {
-              "path": "collection/key",
-              "filters": [],
-              "orderBys": []
-            },
-            "metadata": [
-              [
-                "collection/key",
-                0,
-                {
-                  "foo": "bar"
-                }
-              ]
-            ],
-            "errorCode": 0,
-            "fromCache": false,
-            "hasPendingWrites": false
-          }
-        ]
       }
     ]
   },
-  "Writes that fail with code unknown are retried": {
+  "Writes that fail with code cancelled are retried": {
     "describeName": "Writes:",
-    "itName": "Writes that fail with code unknown are retried",
+    "itName": "Writes that fail with code cancelled are retried",
     "tags": [],
     "config": {
       "useGarbageCollection": true
@@ -4562,7 +4505,7 @@
       {
         "failWrite": {
           "error": {
-            "code": 2
+            "code": 1
           },
           "expectUserCallback": false
         }
@@ -4626,9 +4569,9 @@
       }
     ]
   },
-  "Writes that fail with code deadline-exceeded are retried": {
+  "Writes that fail with code unknown are retried": {
     "describeName": "Writes:",
-    "itName": "Writes that fail with code deadline-exceeded are retried",
+    "itName": "Writes that fail with code unknown are retried",
     "tags": [],
     "config": {
       "useGarbageCollection": true
@@ -4689,7 +4632,7 @@
       {
         "failWrite": {
           "error": {
-            "code": 4
+            "code": 2
           },
           "expectUserCallback": false
         }
@@ -4753,9 +4696,9 @@
       }
     ]
   },
-  "Writes that fail with code resource-exhausted are retried": {
+  "Writes that fail with code deadline-exceeded are retried": {
     "describeName": "Writes:",
-    "itName": "Writes that fail with code resource-exhausted are retried",
+    "itName": "Writes that fail with code deadline-exceeded are retried",
     "tags": [],
     "config": {
       "useGarbageCollection": true
@@ -4816,7 +4759,7 @@
       {
         "failWrite": {
           "error": {
-            "code": 8
+            "code": 4
           },
           "expectUserCallback": false
         }
diff --git a/Firestore/Source/Remote/FSTRemoteStore.m b/Firestore/Source/Remote/FSTRemoteStore.m
index 85608e7..b4a6449 100644
--- a/Firestore/Source/Remote/FSTRemoteStore.m
+++ b/Firestore/Source/Remote/FSTRemoteStore.m
@@ -38,7 +38,15 @@ NS_ASSUME_NONNULL_BEGIN
  * The maximum number of pending writes to allow.
  * TODO(bjornick): Negotiate this value with the backend.
  */
-static const NSUInteger kMaxPendingWrites = 10;
+static const int kMaxPendingWrites = 10;
+
+/**
+ * The FSTRemoteStore notifies an onlineStateDelegate with FSTOnlineStateFailed if we fail to
+ * connect to the backend. This subsequently triggers get() requests to fail or use cached data,
+ * etc. Unfortunately, our connections have historically been subject to various transient failures.
+ * So we wait for multiple failures before notifying the onlineStateDelegate.
+ */
+static const int kOnlineAttemptsBeforeFailure = 2;
 
 #pragma mark - FSTRemoteStore
 
@@ -98,6 +106,9 @@ static const NSUInteger kMaxPendingWrites = 10;
  */
 @property(nonatomic, assign) FSTOnlineState watchStreamOnlineState;
 
+/** A count of consecutive failures to open the stream. */
+@property(nonatomic, assign) int watchStreamFailures;
+
 #pragma mark Write Stream
 // The writeStream is null when the network is disabled. The non-null check is performed by
 // isNetworkEnabled.
@@ -144,6 +155,34 @@ static const NSUInteger kMaxPendingWrites = 10;
   [self enableNetwork];
 }
 
+- (void)setOnlineStateToHealthy {
+  [self updateAndNotifyAboutOnlineState:FSTOnlineStateHealthy];
+}
+
+- (void)setOnlineStateToUnknown {
+  // The state is set to unknown when a healthy stream is closed (e.g. due to a token timeout) or
+  // when we have no active listens and therefore there's no need to start the stream. Assuming
+  // there is (possibly in the future) an active listen, then we will eventually move to state
+  // Online or Failed, but we always want to make at least kOnlineAttemptsBeforeFailure attempts
+  // before failing, so we reset the count here.
+  self.watchStreamFailures = 0;
+  [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown];
+}
+
+- (void)updateOnlineStateAfterFailure {
+  // The first failure after we are successfully connected moves us to the 'Unknown' state. We
+  // then may make multiple attempts (based on kOnlineAttemptsBeforeFailure) before we actually
+  // report failure.
+  if (self.watchStreamOnlineState == FSTOnlineStateHealthy) {
+    [self setOnlineStateToUnknown];
+  } else {
+    self.watchStreamFailures++;
+    if (self.watchStreamFailures >= kOnlineAttemptsBeforeFailure) {
+      [self updateAndNotifyAboutOnlineState:FSTOnlineStateFailed];
+    }
+  }
+}
+
 - (void)updateAndNotifyAboutOnlineState:(FSTOnlineState)watchStreamOnlineState {
   BOOL didChange = (watchStreamOnlineState != self.watchStreamOnlineState);
   self.watchStreamOnlineState = watchStreamOnlineState;
@@ -178,7 +217,7 @@ static const NSUInteger kMaxPendingWrites = 10;
   [self fillWritePipeline];  // This may start the writeStream.
 
   // We move back to the unknown state because we might not want to re-open the stream
-  [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown];
+  [self setOnlineStateToUnknown];
 }
 
 - (void)disableNetwork {
@@ -296,7 +335,7 @@ static const NSUInteger kMaxPendingWrites = 10;
 - (void)watchStreamDidChange:(FSTWatchChange *)change
              snapshotVersion:(FSTSnapshotVersion *)snapshotVersion {
   // Mark the connection as healthy because we got a message from the server.
-  [self updateAndNotifyAboutOnlineState:FSTOnlineStateHealthy];
+  [self setOnlineStateToHealthy];
 
   FSTWatchTargetChange *watchTargetChange =
       [change isKindOfClass:[FSTWatchTargetChange class]] ? (FSTWatchTargetChange *)change : nil;
@@ -334,20 +373,12 @@ static const NSUInteger kMaxPendingWrites = 10;
   // If the watch stream closed due to an error, retry the connection if there are any active
   // watch targets.
   if ([self shouldStartWatchStream]) {
-    // If the connection fails before the stream has become healthy, consider the online state
-    // failed. Otherwise consider the online state unknown and the next connection attempt will
-    // resolve the online state. For example, if a healthy stream is closed due to an expired token
-    // we want to have one more try at reconnecting before we consider the connection unhealthy.
-    if (self.watchStreamOnlineState == FSTOnlineStateHealthy) {
-      [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown];
-    } else {
-      [self updateAndNotifyAboutOnlineState:FSTOnlineStateFailed];
-    }
+    [self updateOnlineStateAfterFailure];
     [self.watchStream start];
   } else {
     // We don't need to restart the watch stream because there are no active targets. The online
     // state is set to unknown because there is no active attempt at establishing a connection.
-    [self updateAndNotifyAboutOnlineState:FSTOnlineStateUnknown];
+    [self setOnlineStateToUnknown];
   }
 }