From 5db88f16617e2ef4c313dba4d5a7f91d31e66c8d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 29 Nov 2017 17:00:03 -0800 Subject: Fixing race in FSTWriteStream --- Firestore/Source/Remote/FSTStream.m | 12 ++++++------ Firestore/Source/Util/FSTDispatchQueue.m | 5 ++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/Firestore/Source/Remote/FSTStream.m b/Firestore/Source/Remote/FSTStream.m index 6cce921..bf54a6b 100644 --- a/Firestore/Source/Remote/FSTStream.m +++ b/Firestore/Source/Remote/FSTStream.m @@ -381,17 +381,17 @@ static const NSTimeInterval kIdleTimeout = 60.0; _messageReceived = NO; _rpc = nil; - // If the caller explicitly requested a stream stop, don't notify them of a closing stream (it - // could trigger undesirable recovery logic, etc.). - if (finalState != FSTStreamStateStopped) { - [self notifyStreamInterruptedWithError:error]; - } - // Clear the delegates to avoid any possible bleed through of events from GRPC. FSTAssert(_delegate, @"closeWithFinalState should only be called for a started stream that has an active " @"delegate."); _delegate = nil; + + // If the caller explicitly requested a stream stop, don't notify them of a closing stream (it + // could trigger undesirable recovery logic, etc.). + if (finalState != FSTStreamStateStopped) { + [self notifyStreamInterruptedWithError:error]; + } } - (void)stop { diff --git a/Firestore/Source/Util/FSTDispatchQueue.m b/Firestore/Source/Util/FSTDispatchQueue.m index 8c953fe..2c8ee55 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.m +++ b/Firestore/Source/Util/FSTDispatchQueue.m @@ -58,7 +58,10 @@ NS_ASSUME_NONNULL_BEGIN - (void)dispatchAfterDelay:(NSTimeInterval)delay block:(void (^)(void))block { dispatch_time_t delayNs = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * NSEC_PER_SEC)); - dispatch_after(delayNs, self.queue, block); + dispatch_after(delayNs, self.queue, ^() { + // Make sure that we prioritize tasks that are already queued for immediate execution. + [self dispatchAsyncAllowingSameQueue:block]; + }); } #pragma mark - Private Methods -- cgit v1.2.3 From df76451b3101c47865ccd5721c96a37ca8c8fa70 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 29 Nov 2017 21:05:46 -0800 Subject: Fixing tests --- Firestore/Source/Remote/FSTStream.m | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/Firestore/Source/Remote/FSTStream.m b/Firestore/Source/Remote/FSTStream.m index 0790d81..9a780e5 100644 --- a/Firestore/Source/Remote/FSTStream.m +++ b/Firestore/Source/Remote/FSTStream.m @@ -347,6 +347,10 @@ static const NSTimeInterval kIdleTimeout = 60.0; [self.workerDispatchQueue verifyIsCurrentQueue]; [self cancelIdleCheck]; + FSTAssert(self.delegate, + @"closeWithFinalState should only be called for a started stream that has an active " + @"delegate."); + if (finalState != FSTStreamStateError) { // If this is an intentional close ensure we don't delay our next connection attempt. [self.backoff reset]; @@ -381,17 +385,14 @@ static const NSTimeInterval kIdleTimeout = 60.0; _messageReceived = NO; _rpc = nil; - // Clear the delegates to avoid any possible bleed through of events from GRPC. - FSTAssert(_delegate, - @"closeWithFinalState should only be called for a started stream that has an active " - @"delegate."); - _delegate = nil; - // If the caller explicitly requested a stream stop, don't notify them of a closing stream (it // could trigger undesirable recovery logic, etc.). if (finalState != FSTStreamStateStopped) { [self notifyStreamInterruptedWithError:error]; } + + // Clear the delegates to avoid any possible bleed through of events from GRPC. + _delegate = nil; } - (void)stop { @@ -615,7 +616,9 @@ static const NSTimeInterval kIdleTimeout = 60.0; } - (void)notifyStreamInterruptedWithError:(nullable NSError *)error { - [self.delegate watchStreamWasInterruptedWithError:error]; + id delegate = self.delegate; + self.delegate = nil; + [delegate watchStreamWasInterruptedWithError:error]; } - (void)watchQuery:(FSTQueryData *)query { @@ -701,7 +704,9 @@ static const NSTimeInterval kIdleTimeout = 60.0; } - (void)notifyStreamInterruptedWithError:(nullable NSError *)error { - [self.delegate writeStreamWasInterruptedWithError:error]; + id delegate = self.delegate; + self.delegate = nil; + [delegate writeStreamWasInterruptedWithError:error]; } - (void)tearDown { -- cgit v1.2.3 From d200fe09efaa57ed7e014e76d1bed52992f7ebc5 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 30 Nov 2017 13:28:36 -0800 Subject: Checking for stream close in handleStreamClose --- Firestore/Source/Remote/FSTStream.m | 13 ++++++++----- Firestore/Source/Util/FSTDispatchQueue.m | 5 +---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Firestore/Source/Remote/FSTStream.m b/Firestore/Source/Remote/FSTStream.m index 9a780e5..ac51cb9 100644 --- a/Firestore/Source/Remote/FSTStream.m +++ b/Firestore/Source/Remote/FSTStream.m @@ -343,14 +343,13 @@ static const NSTimeInterval kIdleTimeout = 60.0; - (void)closeWithFinalState:(FSTStreamState)finalState error:(nullable NSError *)error { FSTAssert(finalState == FSTStreamStateError || error == nil, @"Can't provide an error when not in an error state."); - - [self.workerDispatchQueue verifyIsCurrentQueue]; - [self cancelIdleCheck]; - FSTAssert(self.delegate, @"closeWithFinalState should only be called for a started stream that has an active " @"delegate."); + [self.workerDispatchQueue verifyIsCurrentQueue]; + [self cancelIdleCheck]; + if (finalState != FSTStreamStateError) { // If this is an intentional close ensure we don't delay our next connection attempt. [self.backoff reset]; @@ -516,7 +515,11 @@ static const NSTimeInterval kIdleTimeout = 60.0; */ - (void)handleStreamClose:(nullable NSError *)error { FSTLog(@"%@ %p close: %@", NSStringFromClass([self class]), (__bridge void *)self, error); - FSTAssert([self isStarted], @"Can't handle server close in non-started state."); + + if (![self isStarted]) { // The stream could have already been closed by the idle close timer. + FSTLog(@"%@ Ignoring server close for already closed stream.", NSStringFromClass([self class])); + return; + } // In theory the stream could close cleanly, however, in our current model we never expect this // to happen because if we stop a stream ourselves, this callback will never be called. To diff --git a/Firestore/Source/Util/FSTDispatchQueue.m b/Firestore/Source/Util/FSTDispatchQueue.m index 9ca28a2..6ce5d74 100644 --- a/Firestore/Source/Util/FSTDispatchQueue.m +++ b/Firestore/Source/Util/FSTDispatchQueue.m @@ -58,10 +58,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)dispatchAfterDelay:(NSTimeInterval)delay block:(void (^)(void))block { dispatch_time_t delayNs = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * NSEC_PER_SEC)); - dispatch_after(delayNs, self.queue, ^() { - // Make sure that we prioritize tasks that are already queued for immediate execution. - [self dispatchAsyncAllowingSameQueue:block]; - }); + dispatch_after(delayNs, self.queue, block); } #pragma mark - Private Methods -- cgit v1.2.3 From 47653ce836bb7913cdaf32a4c2bea0b23008e584 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 1 Dec 2017 11:25:49 -0800 Subject: Adding Changelog entry --- Firestore/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index 0aba04e..483146b 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,5 +1,7 @@ # Unreleased +- [fixed] Addressed race condition during the teardown of idle streams (#490). + # v0.9.3 - [changed] Improved performance loading documents matching a query. - [changed] Cleanly shut down idle write streams. -- cgit v1.2.3