firebase · mikelehen · Sep 6, 2018 · Sep 6, 2018
diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md
@@ -1,4 +1,11 @@
 # Unreleased
+- [fixed] Fixed an issue where the first `get()` call made after being offline
+  could incorrectly return cached data without attempting to reach the backend.
+- [changed] Changed `get()` to only make 1 attempt to reach the backend before
+  returning cached data, potentially reducing delays while offline. Previously
+  it would make 2 attempts, to work around a backend bug.
+
+# 17.1.0
 - [feature] Added `FieldValue.arrayUnion()` and `FieldValue.arrayRemove()` to
   atomically add and remove elements from an array field in a document.
 - [feature] Added `Query.whereArrayContains()` query operator to find documents

diff --git a/...base-firestore/src/main/java/com/google/firebase/firestore/remote/OnlineStateTracker.java b/...base-firestore/src/main/java/com/google/firebase/firestore/remote/OnlineStateTracker.java
@@ -46,7 +46,10 @@ interface OnlineStateCallback {
 
   // To deal with transient failures, we allow multiple stream attempts before giving up and
   // transitioning from OnlineState.UNKNOWN to OFFLINE.
-  private static final int MAX_WATCH_STREAM_FAILURES = 2;
+  // TODO(mikelehen): This used to be set to 2 as a mitigation for b/66228394. @jdimond thinks that
+  // bug is sufficiently fixed so that we can set this back to 1. If that works okay, we could
+  // potentially remove this logic entirely.
+  private static final int MAX_WATCH_STREAM_FAILURES = 1;
 
   // To deal with stream attempts that don't succeed or fail in a timely manner, we have a
   // timeout for OnlineState to reach ONLINE or OFFLINE. If the timeout is reached, we transition

diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java
@@ -325,8 +325,16 @@ public void stopListening(int targetId) {
     // The watch stream might not be started if we're in a disconnected state
     if (watchStream.isOpen()) {
       sendUnwatchRequest(targetId);
-      if (listenTargets.isEmpty()) {
+    }
+
+    if (listenTargets.isEmpty()) {
+      if (watchStream.isOpen()) {
         watchStream.markIdle();
+      } else if (this.canUseNetwork()) {
+        // Revert to OnlineState.UNKNOWN if the watch stream is not open and we have no listeners,
+        // since without any listens to send we cannot confirm if the stream is healthy and upgrade
+        // to OnlineState.ONLINE.
+        this.onlineStateTracker.updateState(OnlineState.UNKNOWN);
       }
     }
   }

diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java
@@ -133,6 +133,7 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
           : Sets.newHashSet("no-android", "benchmark", "multi-client");
 
   private boolean garbageCollectionEnabled;
+  private boolean networkEnabled = true;
 
   //
   // Parts of the Firestore system that the spec tests need to control.
@@ -670,6 +671,7 @@ private void doDrainQueue() throws Exception {
   }
 
   private void doDisableNetwork() throws Exception {
+    networkEnabled = false;
     queue.runSync(
         () -> {
           // Make sure to execute all writes that are currently queued. This allows us
@@ -680,6 +682,7 @@ private void doDisableNetwork() throws Exception {
   }
 
   private void doEnableNetwork() throws Exception {
+    networkEnabled = true;
     queue.runSync(() -> remoteStore.enableNetwork());
   }
 
@@ -939,6 +942,10 @@ private void validateLimboDocs() {
   }
 
   private void validateActiveTargets() {
+    if (!networkEnabled) {
+      return;
+    }
+
     // Create a copy so we can modify it in tests
     Map<Integer, QueryData> actualTargets = new HashMap<>(datastore.activeTargets());
 

diff --git a/firebase-firestore/src/test/resources/json/offline_spec_test.json b/firebase-firestore/src/test/resources/json/offline_spec_test.json
@@ -2,10 +2,7 @@
   "Empty queries are resolved if client goes offline": {
     "describeName": "Offline:",
     "itName": "Empty queries are resolved if client goes offline",
-    "tags": [
-      "no-android",
-      "no-ios"
-    ],
+    "tags": [],
     "config": {
       "useGarbageCollection": true,
       "numClients": 1
@@ -77,10 +74,7 @@
   "A successful message delays offline status": {
     "describeName": "Offline:",
     "itName": "A successful message delays offline status",
-    "tags": [
-      "no-android",
-      "no-ios"
-    ],
+    "tags": [],
     "config": {
       "useGarbageCollection": true,
       "numClients": 1
@@ -167,9 +161,7 @@
     "describeName": "Offline:",
     "itName": "Removing all listeners delays \"Offline\" status on next listen",
     "tags": [
-      "eager-gc",
-      "no-android",
-      "no-ios"
+      "eager-gc"
     ],
     "comment": "Marked as no-lru because when a listen is re-added, it gets a new target id rather than reusing one",
     "config": {
@@ -290,10 +282,7 @@
   "Queries revert to fromCache=true when offline.": {
     "describeName": "Offline:",
     "itName": "Queries revert to fromCache=true when offline.",
-    "tags": [
-      "no-android",
-      "no-ios"
-    ],
+    "tags": [],
     "config": {
       "useGarbageCollection": true,
       "numClients": 1
@@ -461,10 +450,7 @@
   "Queries with limbo documents handle going offline.": {
     "describeName": "Offline:",
     "itName": "Queries with limbo documents handle going offline.",
-    "tags": [
-      "no-android",
-      "no-ios"
-    ],
+    "tags": [],
     "config": {
       "useGarbageCollection": true,
       "numClients": 1
@@ -889,10 +875,7 @@
   "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": [
-      "no-android",
-      "no-ios"
-    ],
+    "tags": [],
     "config": {
       "useGarbageCollection": true,
       "numClients": 1
@@ -1074,5 +1057,117 @@
         ]
       }
     ]
+  },
+  "Queries return from cache when network disabled": {
+    "describeName": "Offline:",
+    "itName": "Queries return from cache when network disabled",
+    "tags": ["eager-gc"],
+    "config": {
+      "useGarbageCollection": true,
+      "numClients": 1
+    },
+    "steps": [
+      {
+        "enableNetwork": false,
+        "stateExpect": {
+          "activeTargets": {},
+          "limboDocs": []
+        }
+      },
+      {
+        "userListen": [
+          2,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {
+            "2": {
+              "query": {
+                "path": "collection",
+                "filters": [],
+                "orderBys": []
+              },
+              "resumeToken": ""
+            }
+          }
+        },
+        "expect": [
+          {
+            "query": {
+              "path": "collection",
+              "filters": [],
+              "orderBys": []
+            },
+            "errorCode": 0,
+            "fromCache": true,
+            "hasPendingWrites": false
+          }
+        ]
+      },
+      {
+        "userUnlisten": [
+          2,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {}
+        }
+      },
+      {
+        "userListen": [
+          4,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {
+            "4": {
+              "query": {
+                "path": "collection",
+                "filters": [],
+                "orderBys": []
+              },
+              "resumeToken": ""
+            }
+          }
+        },
+        "expect": [
+          {
+            "query": {
+              "path": "collection",
+              "filters": [],
+              "orderBys": []
+            },
+            "errorCode": 0,
+            "fromCache": true,
+            "hasPendingWrites": false
+          }
+        ]
+      },
+      {
+        "userUnlisten": [
+          4,
+          {
+            "path": "collection",
+            "filters": [],
+            "orderBys": []
+          }
+        ],
+        "stateExpect": {
+          "activeTargets": {}
+        }
+      }
+    ]
   }
 }
diff --git a/firebase-firestore/src/test/resources/json/remote_store_spec_test.json b/firebase-firestore/src/test/resources/json/remote_store_spec_test.json
@@ -482,10 +482,7 @@
   "Cleans up watch state correctly": {
     "describeName": "Remote store:",
     "itName": "Cleans up watch state correctly",
-    "tags": [
-      "no-android",
-      "no-ios"
-    ],
+    "tags": [],
     "config": {
       "useGarbageCollection": false,
       "numClients": 1