From 5f49b2f3f9866e4db13d09857eb3b548239cc62e Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Wed, 21 Mar 2018 14:59:49 -0700 Subject: Fix for b/74749605: Cancel pending backoff operations when closing streams. (#958) Port of https://github.com/firebase/firebase-js-sdk/pull/564. --- Firestore/Example/Tests/SpecTests/FSTSpecTests.mm | 5 ++ .../SpecTests/json/existence_filter_spec_test.json | 3 +- .../Tests/SpecTests/json/offline_spec_test.json | 69 +++++++++++++------- .../SpecTests/json/remote_store_spec_test.json | 74 +++++++++++++++++++++- .../SpecTests/json/resume_token_spec_test.json | 3 +- Firestore/Source/Remote/FSTExponentialBackoff.h | 3 + Firestore/Source/Remote/FSTExponentialBackoff.mm | 12 +++- Firestore/Source/Remote/FSTStream.mm | 9 ++- Firestore/Source/Util/FSTDispatchQueue.mm | 7 +- 9 files changed, 150 insertions(+), 35 deletions(-) diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm index 1e16250..0c3d9a1 100644 --- a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm +++ b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm @@ -296,6 +296,11 @@ static NSString *const kNoIOSTag = @"no-ios"; - (void)doWatchStreamClose:(NSDictionary *)closeSpec { NSDictionary *errorSpec = closeSpec[@"error"]; int code = ((NSNumber *)(errorSpec[@"code"])).intValue; + + NSNumber *runBackoffTimer = closeSpec[@"runBackoffTimer"]; + // TODO(b/72313632): Incorporate backoff in iOS Spec Tests. + FSTAssert(runBackoffTimer.boolValue, @"iOS Spec Tests don't support backoff."); + [self.driver receiveWatchStreamError:code userInfo:errorSpec]; } diff --git a/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json b/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json index ab42241..abd2cf4 100644 --- a/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json @@ -337,7 +337,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "stateExpect": { "activeTargets": { diff --git a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json index fc9f295..db8324b 100644 --- a/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/offline_spec_test.json @@ -34,7 +34,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -42,7 +43,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "expect": [ { @@ -62,7 +64,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -70,7 +73,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } } ] @@ -115,7 +119,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -123,7 +128,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -131,7 +137,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "expect": [ { @@ -151,7 +158,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -159,7 +167,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } } ] @@ -199,7 +208,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -207,7 +217,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "expect": [ { @@ -240,7 +251,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -270,7 +282,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -278,7 +291,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "expect": [ { @@ -381,7 +395,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "stateExpect": { "activeTargets": { @@ -401,7 +416,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -409,7 +425,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "expect": [ { @@ -595,7 +612,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "stateExpect": { "activeTargets": { @@ -623,7 +641,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -631,7 +650,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -764,7 +784,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -772,7 +793,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -834,7 +856,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "stateExpect": { "activeTargets": { 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 26bb520..6852c90 100644 --- a/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/remote_store_spec_test.json @@ -488,7 +488,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true } }, { @@ -543,5 +544,76 @@ ] } ] + }, + "Handles user changes while offline (b/74749605).": { + "describeName": "Remote store:", + "itName": "Handles user changes while offline (b/74749605).", + "tags": [ + "no-android", + "no-ios" + ], + "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": false + }, + "stateExpect": { + "activeTargets": {} + } + }, + { + "changeUser": "abc", + "stateExpect": { + "activeTargets": { + "2": { + "query": { + "path": "collection", + "filters": [], + "orderBys": [] + }, + "resumeToken": "" + } + } + } + }, + { + "watchStreamClose": { + "error": { + "code": 14, + "message": "Simulated Backend Error" + }, + "runBackoffTimer": true + } + } + ] } } diff --git a/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json b/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json index 25ea84a..f411d98 100644 --- a/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/resume_token_spec_test.json @@ -85,7 +85,8 @@ "error": { "code": 14, "message": "Simulated Backend Error" - } + }, + "runBackoffTimer": true }, "stateExpect": { "activeTargets": { diff --git a/Firestore/Source/Remote/FSTExponentialBackoff.h b/Firestore/Source/Remote/FSTExponentialBackoff.h index 03558ee..3beafbf 100644 --- a/Firestore/Source/Remote/FSTExponentialBackoff.h +++ b/Firestore/Source/Remote/FSTExponentialBackoff.h @@ -76,6 +76,9 @@ NS_ASSUME_NONNULL_BEGIN */ - (void)backoffAndRunBlock:(void (^)(void))block; +/** Cancels any pending backoff block scheduled via backoffAndRunBlock:. */ +- (void)cancel; + @end NS_ASSUME_NONNULL_END diff --git a/Firestore/Source/Remote/FSTExponentialBackoff.mm b/Firestore/Source/Remote/FSTExponentialBackoff.mm index dddf164..20b50a5 100644 --- a/Firestore/Source/Remote/FSTExponentialBackoff.mm +++ b/Firestore/Source/Remote/FSTExponentialBackoff.mm @@ -66,9 +66,8 @@ using firebase::firestore::util::SecureRandom; } - (void)backoffAndRunBlock:(void (^)(void))block { - if (self.timerCallback) { - [self.timerCallback cancel]; - } + [self cancel]; + // First schedule the block using the current base (which may be 0 and should be honored as such). NSTimeInterval delayWithJitter = _currentBase + [self jitterDelay]; if (_currentBase > 0) { @@ -89,6 +88,13 @@ using firebase::firestore::util::SecureRandom; } } +- (void)cancel { + if (self.timerCallback) { + [self.timerCallback cancel]; + self.timerCallback = nil; + } +} + /** Returns a random value in the range [-currentBase/2, currentBase/2] */ - (NSTimeInterval)jitterDelay { std::uniform_real_distribution dist; diff --git a/Firestore/Source/Remote/FSTStream.mm b/Firestore/Source/Remote/FSTStream.mm index a5c36c8..f65230b 100644 --- a/Firestore/Source/Remote/FSTStream.mm +++ b/Firestore/Source/Remote/FSTStream.mm @@ -326,7 +326,8 @@ static const NSTimeInterval kIdleTimeout = 60.0; /** Resumes stream start after backing off. */ - (void)resumeStartFromBackoffWithDelegate:(id)delegate { if (self.state == FSTStreamStateStopped) { - // Streams can be stopped while waiting for backoff to complete. + // We should have canceled the backoff timer when the stream was closed, but just in case we + // make this a no-op. return; } @@ -367,8 +368,14 @@ static const NSTimeInterval kIdleTimeout = 60.0; @"Can't provide an error when not in an error state."); [self.workerDispatchQueue verifyIsCurrentQueue]; + + // The stream will be closed so we don't need our idle close timer anymore. [self cancelIdleCheck]; + // Ensure we don't leave a pending backoff operation queued (in case close() + // was called while we were waiting to reconnect). + [self.backoff cancel]; + if (finalState != FSTStreamStateError) { // If this is an intentional close ensure we don't delay our next connection attempt. [self.backoff reset]; diff --git a/Firestore/Source/Util/FSTDispatchQueue.mm b/Firestore/Source/Util/FSTDispatchQueue.mm index 52482df..15d6e7b 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.mm +++ b/Firestore/Source/Util/FSTDispatchQueue.mm @@ -230,11 +230,8 @@ NS_ASSUME_NONNULL_BEGIN block:(void (^)(void))block { // While not necessarily harmful, we currently don't expect to have multiple callbacks with the // same timerID in the queue, so defensively reject them. - // TODO(b/74749605): If a user change happens while offline we can end up with multiple backoff - // callbacks in the dispatch queue. This is non-harmful so I'm just disabling the assert until we - // get that cleaned up. - // FSTAssert(![self containsDelayedCallbackWithTimerID:timerID], - // @"Attempted to schedule multiple callbacks with id %ld", (unsigned long)timerID); + FSTAssert(![self containsDelayedCallbackWithTimerID:timerID], + @"Attempted to schedule multiple callbacks with id %ld", (unsigned long)timerID); FSTDelayedCallback *delayedCallback = [FSTDelayedCallback createAndScheduleWithQueue:self timerID:timerID delay:delay -- cgit v1.2.3