Implement global resume token (#1696)

Author: wilhuff

Firestore/CHANGELOG.md

@@ -1,6 +1,8 @@
 # Unreleased
 - [fixed] Fixed an issue where changes to custom authentication claims did not
   take effect until you did a full sign-out and sign-in. (#1499)
+- [changed] Improved how Firestore handles idle queries to reduce the cost of
+  re-listening within 30 minutes.
 
 # v0.13.1
 - [fixed] Fixed an issue where `get(source:.Cache)` could throw an

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

@@ -78,6 +78,10 @@
 
 NSString *const kNoLRUTag = @"no-lru";
 
+static NSString *Describe(NSData *data) {
+  return [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
+}
+
 @interface FSTSpecTests ()
 @property(nonatomic, strong) FSTSyncEngineTestDriver *driver;
 
@@ -660,7 +664,7 @@ - (void)validateActiveTargets {
       XCTAssertEqualObjects(actual.query, queryData.query);
       XCTAssertEqual(actual.targetID, queryData.targetID);
       XCTAssertEqual(actual.snapshotVersion, queryData.snapshotVersion);
-      XCTAssertEqualObjects(actual.resumeToken, queryData.resumeToken);
+      XCTAssertEqualObjects(Describe(actual.resumeToken), Describe(queryData.resumeToken));
     }
 
     [actualTargets removeObjectForKey:targetID];

Firestore/Example/Tests/SpecTests/json/listen_spec_test.json

@@ -4060,7 +4060,7 @@
   "Persists global resume tokens on unlisten": {
     "describeName": "Listens:",
     "itName": "Persists global resume tokens on unlisten",
-    "tags": ["no-ios"],
+    "tags": [],
     "config": {
       "useGarbageCollection": false,
       "numClients": 1
@@ -4248,7 +4248,7 @@
   "Omits global resume tokens for a short while": {
     "describeName": "Listens:",
     "itName": "Omits global resume tokens for a short while",
-    "tags": ["no-ios"],
+    "tags": [],
     "config": {
       "useGarbageCollection": false,
       "numClients": 1
@@ -4423,7 +4423,7 @@
   "Persists global resume tokens if the snapshot is old enough": {
     "describeName": "Listens:",
     "itName": "Persists global resume tokens if the snapshot is old enough",
-    "tags": ["no-ios"],
+    "tags": [],
     "config": {
       "useGarbageCollection": false,
       "numClients": 1

Firestore/Source/Local/FSTLocalStore.mm

@@ -53,6 +53,14 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+/**
+ * The maximum time to leave a resume token buffered without writing it out. This value is
+ * arbitrary: it's long enough to avoid several writes (possibly indefinitely if updates come more
+ * frequently than this) but short enough that restarting after crashing will still have a pretty
+ * recent resume token.
+ */
+static const int64_t kResumeTokenMaxAgeSeconds = 5 * 60;  // 5 minutes
+
 @interface FSTLocalStore ()
 
 /** Manages our in-memory or durable persistence. */
@@ -277,11 +285,15 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
       // than documents that were previously removed from this target.
       NSData *resumeToken = change.resumeToken;
       if (resumeToken.length > 0) {
+        FSTQueryData *oldQueryData = queryData;
         queryData = [queryData queryDataByReplacingSnapshotVersion:remoteEvent.snapshotVersion
                                                        resumeToken:resumeToken
                                                     sequenceNumber:sequenceNumber];
         self.targetIDs[boxedTargetID] = queryData;
-        [self.queryCache updateQueryData:queryData];
+
+        if ([self shouldPersistQueryData:queryData oldQueryData:oldQueryData change:change]) {
+          [self.queryCache updateQueryData:queryData];
+        }
       }
     }
 
@@ -338,6 +350,43 @@ - (FSTMaybeDocumentDictionary *)applyRemoteEvent:(FSTRemoteEvent *)remoteEvent {
   });
 }
 
+/**
+ * Returns YES if the newQueryData should be persisted during an update of an active target.
+ * QueryData should always be persisted when a target is being released and should not call this
+ * function.
+ *
+ * While the target is active, QueryData updates can be omitted when nothing about the target has
+ * changed except metadata like the resume token or snapshot version. Occasionally it's worth the
+ * extra write to prevent these values from getting too stale after a crash, but this doesn't have
+ * to be too frequent.
+ */
+- (BOOL)shouldPersistQueryData:(FSTQueryData *)newQueryData
+                  oldQueryData:(FSTQueryData *)oldQueryData
+                        change:(FSTTargetChange *)change {
+  // Avoid clearing any existing value
+  if (newQueryData.resumeToken.length == 0) return NO;
+
+  // Any resume token is interesting if there isn't one already.
+  if (oldQueryData.resumeToken.length == 0) return YES;
+
+  // Don't allow resume token changes to be buffered indefinitely. This allows us to be reasonably
+  // up-to-date after a crash and avoids needing to loop over all active queries on shutdown.
+  // Especially in the browser we may not get time to do anything interesting while the current
+  // tab is closing.
+  int64_t newSeconds = newQueryData.snapshotVersion.timestamp().seconds();
+  int64_t oldSeconds = oldQueryData.snapshotVersion.timestamp().seconds();
+  int64_t timeDelta = newSeconds - oldSeconds;
+  if (timeDelta >= kResumeTokenMaxAgeSeconds) return YES;
+
+  // Otherwise if the only thing that has changed about a target is its resume token then it's not
+  // worth persisting. Note that the RemoteStore keeps an in-memory view of the currently active
+  // targets which includes the current resume token, so stream failure or user changes will still
+  // use an up-to-date resume token regardless of what we do here.
+  size_t changes = change.addedDocuments.size() + change.modifiedDocuments.size() +
+                   change.removedDocuments.size();
+  return changes > 0;
+}
+
 - (void)notifyLocalViewChanges:(NSArray<FSTLocalViewChanges *> *)viewChanges {
   self.persistence.run("NotifyLocalViewChanges", [&]() {
     FSTReferenceSet *localViewReferences = self.localViewReferences;
@@ -391,9 +440,21 @@ - (void)releaseQuery:(FSTQuery *)query {
     FSTQueryData *queryData = [self.queryCache queryDataForQuery:query];
     HARD_ASSERT(queryData, "Tried to release nonexistent query: %s", query);
 
-    [self.localViewReferences removeReferencesForID:queryData.targetID];
+    TargetId targetID = queryData.targetID;
+    FSTBoxedTargetID *boxedTargetID = @(targetID);
+
+    FSTQueryData *cachedQueryData = self.targetIDs[boxedTargetID];
+    if (cachedQueryData.snapshotVersion > queryData.snapshotVersion) {
+      // If we've been avoiding persisting the resumeToken (see shouldPersistQueryData for
+      // conditions and rationale) we need to persist the token now because there will no
+      // longer be an in-memory version to fall back on.
+      queryData = cachedQueryData;
+      [self.queryCache updateQueryData:queryData];
+    }
+
+    [self.localViewReferences removeReferencesForID:targetID];
+    [self.targetIDs removeObjectForKey:boxedTargetID];
     [self.persistence.referenceDelegate removeTarget:queryData];
-    [self.targetIDs removeObjectForKey:@(queryData.targetID)];
 
     // If this was the last watch target, then we won't get any more watch snapshots, so we should
     // release any held batch results.

Firestore/Source/Remote/FSTRemoteEvent.mm

@@ -20,6 +20,7 @@
 #include <set>
 #include <unordered_map>
 #include <utility>
+#include <vector>
 
 #import "Firestore/Source/Core/FSTQuery.h"
 #import "Firestore/Source/Core/FSTViewSnapshot.h"
@@ -339,8 +340,7 @@ - (void)handleDocumentChange:(FSTDocumentWatchChange *)documentChange {
 }
 
 - (void)handleTargetChange:(FSTWatchTargetChange *)targetChange {
-  for (FSTBoxedTargetID *boxedTargetID in targetChange.targetIDs) {
-    int targetID = boxedTargetID.intValue;
+  for (TargetId targetID : [self targetIdsForChange:targetChange]) {
     FSTTargetState *targetState = [self ensureTargetStateForTarget:targetID];
     switch (targetChange.state) {
       case FSTWatchTargetChangeStateNoChange:
@@ -388,6 +388,27 @@ - (void)handleTargetChange:(FSTWatchTargetChange *)targetChange {
   }
 }
 
+/**
+ * Returns all targetIds that the watch change applies to: either the targetIds explicitly listed
+ * in the change or the targetIds of all currently active targets.
+ */
+- (std::vector<TargetId>)targetIdsForChange:(FSTWatchTargetChange *)targetChange {
+  NSArray<NSNumber *> *targetIDs = targetChange.targetIDs;
+  std::vector<TargetId> result;
+  if (targetIDs.count > 0) {
+    result.reserve(targetIDs.count);
+    for (NSNumber *targetID in targetIDs) {
+      result.push_back(targetID.intValue);
+    }
+  } else {
+    result.reserve(_targetStates.size());
+    for (const auto &entry : _targetStates) {
+      result.push_back(entry.first);
+    }
+  }
+  return result;
+}
+
 - (void)removeTarget:(TargetId)targetID {
   _targetStates.erase(targetID);
 }