diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index e05ed0becf4..63ea72f9ef1 100644 --- a/Firestore/CHANGELOG.md +++ b/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 diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index fccec3b6fb2..f8ee63bd5db 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/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]; diff --git a/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json b/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json index 3a43466fb3c..54144a6b610 100644 --- a/Firestore/Example/Tests/SpecTests/json/listen_spec_test.json +++ b/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 diff --git a/Firestore/Source/Local/FSTLocalStore.mm b/Firestore/Source/Local/FSTLocalStore.mm index cb44067c68e..89bf6e2e5ae 100644 --- a/Firestore/Source/Local/FSTLocalStore.mm +++ b/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 *)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. diff --git a/Firestore/Source/Remote/FSTRemoteEvent.mm b/Firestore/Source/Remote/FSTRemoteEvent.mm index e578e8c6c68..ee674b372b5 100644 --- a/Firestore/Source/Remote/FSTRemoteEvent.mm +++ b/Firestore/Source/Remote/FSTRemoteEvent.mm @@ -20,6 +20,7 @@ #include #include #include +#include #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)targetIdsForChange:(FSTWatchTargetChange *)targetChange { + NSArray *targetIDs = targetChange.targetIDs; + std::vector 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); }