From 64cdaae02e382de63692fc1b17b823f07174e47c Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 17 Jan 2019 10:40:18 -0700 Subject: [PATCH 1/3] schedule retry timer on main run loop --- SignalServiceKit/src/Util/NSTimer+OWS.h | 8 +++- SignalServiceKit/src/Util/NSTimer+OWS.m | 20 +++++++++- SignalServiceKit/src/Util/OWSOperation.m | 47 +++++++++++++++--------- 3 files changed, 56 insertions(+), 19 deletions(-) diff --git a/SignalServiceKit/src/Util/NSTimer+OWS.h b/SignalServiceKit/src/Util/NSTimer+OWS.h index e301b39b0..9aa7bebed 100644 --- a/SignalServiceKit/src/Util/NSTimer+OWS.h +++ b/SignalServiceKit/src/Util/NSTimer+OWS.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // NS_ASSUME_NONNULL_BEGIN @@ -14,6 +14,12 @@ NS_ASSUME_NONNULL_BEGIN userInfo:(nullable id)userInfo repeats:(BOOL)repeats; ++ (NSTimer *)weakTimerWithTimeInterval:(NSTimeInterval)timeInterval + target:(id)target + selector:(SEL)selector + userInfo:(nullable id)userInfo + repeats:(BOOL)repeats; + @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Util/NSTimer+OWS.m b/SignalServiceKit/src/Util/NSTimer+OWS.m index a2f02f2a4..9b4ad739b 100644 --- a/SignalServiceKit/src/Util/NSTimer+OWS.m +++ b/SignalServiceKit/src/Util/NSTimer+OWS.m @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // #import "NSTimer+OWS.h" @@ -64,6 +64,24 @@ static void *kNSTimer_OWS_Proxy = &kNSTimer_OWS_Proxy; return timer; } ++ (NSTimer *)weakTimerWithTimeInterval:(NSTimeInterval)timeInterval + target:(id)target + selector:(SEL)selector + userInfo:(nullable id)userInfo + repeats:(BOOL)repeats +{ + NSTimerProxy *proxy = [NSTimerProxy new]; + proxy.target = target; + proxy.selector = selector; + NSTimer *timer = [NSTimer timerWithTimeInterval:timeInterval + target:proxy + selector:@selector(timerFired:) + userInfo:userInfo + repeats:repeats]; + [timer ows_setProxy:proxy]; + return timer; +} + @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index e3455a5f7..e0c0e65a5 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // #import "OWSOperation.h" @@ -131,18 +131,20 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; - (void)runAnyQueuedRetry { - __block NSTimer *_Nullable retryTimer; - dispatch_sync(self.retryTimerSerialQueue, ^{ - retryTimer = self.retryTimer; - self.retryTimer = nil; - [retryTimer invalidate]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + __block NSTimer *_Nullable retryTimer; + dispatch_sync(self.retryTimerSerialQueue, ^{ + retryTimer = self.retryTimer; + self.retryTimer = nil; + [retryTimer invalidate]; + }); + + if (retryTimer != nil) { + [self run]; + } else { + OWSLogVerbose(@"not re-running since operation is already running."); + } }); - - if (retryTimer != nil) { - [self run]; - } else { - OWSLogVerbose(@"not re-running since operation is already running."); - } } #pragma mark - Public Methods @@ -193,11 +195,22 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; dispatch_sync(self.retryTimerSerialQueue, ^{ OWSAssertDebug(self.retryTimer == nil); [self.retryTimer invalidate]; - self.retryTimer = [NSTimer weakScheduledTimerWithTimeInterval:self.retryInterval - target:self - selector:@selector(runAnyQueuedRetry) - userInfo:nil - repeats:NO]; + NSTimer *retryTimer = [NSTimer weakTimerWithTimeInterval:self.retryInterval + target:self + selector:@selector(runAnyQueuedRetry) + userInfo:nil + repeats:NO]; + + self.retryTimer = retryTimer; + + // The `scheduledTimerWith*` methods add the timer to the current thread's RunLoop. + // Since Operations typically run on a background thread, that would mean the background + // thread's RunLoop. However, the OS can spin down background threads if there's no work + // being done, so we run the risk of the timer's RunLoop being deallocated before it's + // fired. + // + // To ensure the timer's thread sticks around, we schedule it on the main RunLoop. + [NSRunLoop.mainRunLoop addTimer:retryTimer forMode:NSDefaultRunLoopMode]; }); } From 323249baa0f12642553feaa4cecd84b00cbeb463 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 17 Jan 2019 11:00:08 -0700 Subject: [PATCH 2/3] NSRunLoop methods should only be accessed on it's corresponding thread. --- SignalServiceKit/src/Util/NSTimer+OWS.h | 6 ---- SignalServiceKit/src/Util/NSTimer+OWS.m | 18 ------------ SignalServiceKit/src/Util/OWSOperation.m | 36 +++++++++++------------- 3 files changed, 17 insertions(+), 43 deletions(-) diff --git a/SignalServiceKit/src/Util/NSTimer+OWS.h b/SignalServiceKit/src/Util/NSTimer+OWS.h index 9aa7bebed..c530f209e 100644 --- a/SignalServiceKit/src/Util/NSTimer+OWS.h +++ b/SignalServiceKit/src/Util/NSTimer+OWS.h @@ -14,12 +14,6 @@ NS_ASSUME_NONNULL_BEGIN userInfo:(nullable id)userInfo repeats:(BOOL)repeats; -+ (NSTimer *)weakTimerWithTimeInterval:(NSTimeInterval)timeInterval - target:(id)target - selector:(SEL)selector - userInfo:(nullable id)userInfo - repeats:(BOOL)repeats; - @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Util/NSTimer+OWS.m b/SignalServiceKit/src/Util/NSTimer+OWS.m index 9b4ad739b..12eb39175 100644 --- a/SignalServiceKit/src/Util/NSTimer+OWS.m +++ b/SignalServiceKit/src/Util/NSTimer+OWS.m @@ -64,24 +64,6 @@ static void *kNSTimer_OWS_Proxy = &kNSTimer_OWS_Proxy; return timer; } -+ (NSTimer *)weakTimerWithTimeInterval:(NSTimeInterval)timeInterval - target:(id)target - selector:(SEL)selector - userInfo:(nullable id)userInfo - repeats:(BOOL)repeats -{ - NSTimerProxy *proxy = [NSTimerProxy new]; - proxy.target = target; - proxy.selector = selector; - NSTimer *timer = [NSTimer timerWithTimeInterval:timeInterval - target:proxy - selector:@selector(timerFired:) - userInfo:userInfo - repeats:repeats]; - [timer ows_setProxy:proxy]; - return timer; -} - @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index e0c0e65a5..df6daf11f 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -192,25 +192,23 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; self.remainingRetries--; - dispatch_sync(self.retryTimerSerialQueue, ^{ - OWSAssertDebug(self.retryTimer == nil); - [self.retryTimer invalidate]; - NSTimer *retryTimer = [NSTimer weakTimerWithTimeInterval:self.retryInterval - target:self - selector:@selector(runAnyQueuedRetry) - userInfo:nil - repeats:NO]; - - self.retryTimer = retryTimer; - - // The `scheduledTimerWith*` methods add the timer to the current thread's RunLoop. - // Since Operations typically run on a background thread, that would mean the background - // thread's RunLoop. However, the OS can spin down background threads if there's no work - // being done, so we run the risk of the timer's RunLoop being deallocated before it's - // fired. - // - // To ensure the timer's thread sticks around, we schedule it on the main RunLoop. - [NSRunLoop.mainRunLoop addTimer:retryTimer forMode:NSDefaultRunLoopMode]; + dispatch_async(dispatch_get_main_queue(), ^{ + dispatch_sync(self.retryTimerSerialQueue, ^{ + OWSAssertDebug(self.retryTimer == nil); + [self.retryTimer invalidate]; + // The `scheduledTimerWith*` methods add the timer to the current thread's RunLoop. + // Since Operations typically run on a background thread, that would mean the background + // thread's RunLoop. However, the OS can spin down background threads if there's no work + // being done, so we run the risk of the timer's RunLoop being deallocated before it's + // fired. + // + // To ensure the timer's thread sticks around, we schedule it while on the main RunLoop. + self.retryTimer = [NSTimer weakScheduledTimerWithTimeInterval:self.retryInterval + target:self + selector:@selector(runAnyQueuedRetry) + userInfo:nil + repeats:NO]; + }); }); } From 343c7d8ece77080392bc256c981427b1775ba89f Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 17 Jan 2019 14:45:43 -0500 Subject: [PATCH 3/3] Remove timer queue. --- SignalServiceKit/src/Util/OWSOperation.m | 50 ++++++++++++------------ 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index df6daf11f..444d9c886 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -18,8 +18,9 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; @property (nullable) NSError *failingError; @property (atomic) OWSOperationState operationState; @property (nonatomic) OWSBackgroundTask *backgroundTask; + +// This property should only be accessed on the main queue. @property (nonatomic) NSTimer *_Nullable retryTimer; -@property (nonatomic, readonly) dispatch_queue_t retryTimerSerialQueue; @end @@ -34,7 +35,6 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; _operationState = OWSOperationStateNew; _backgroundTask = [OWSBackgroundTask backgroundTaskWithLabel:self.logTag]; - _retryTimerSerialQueue = dispatch_queue_create("SignalServiceKit.OWSOperation.retryTimer", DISPATCH_QUEUE_SERIAL); // Operations are not retryable by default. _remainingRetries = 0; @@ -131,16 +131,15 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; - (void)runAnyQueuedRetry { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - __block NSTimer *_Nullable retryTimer; - dispatch_sync(self.retryTimerSerialQueue, ^{ - retryTimer = self.retryTimer; - self.retryTimer = nil; - [retryTimer invalidate]; - }); + dispatch_async(dispatch_get_main_queue(), ^{ + NSTimer *_Nullable retryTimer = self.retryTimer; + self.retryTimer = nil; + [retryTimer invalidate]; if (retryTimer != nil) { - [self run]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + [self run]; + }); } else { OWSLogVerbose(@"not re-running since operation is already running."); } @@ -193,22 +192,21 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; self.remainingRetries--; dispatch_async(dispatch_get_main_queue(), ^{ - dispatch_sync(self.retryTimerSerialQueue, ^{ - OWSAssertDebug(self.retryTimer == nil); - [self.retryTimer invalidate]; - // The `scheduledTimerWith*` methods add the timer to the current thread's RunLoop. - // Since Operations typically run on a background thread, that would mean the background - // thread's RunLoop. However, the OS can spin down background threads if there's no work - // being done, so we run the risk of the timer's RunLoop being deallocated before it's - // fired. - // - // To ensure the timer's thread sticks around, we schedule it while on the main RunLoop. - self.retryTimer = [NSTimer weakScheduledTimerWithTimeInterval:self.retryInterval - target:self - selector:@selector(runAnyQueuedRetry) - userInfo:nil - repeats:NO]; - }); + OWSAssertDebug(self.retryTimer == nil); + [self.retryTimer invalidate]; + + // The `scheduledTimerWith*` methods add the timer to the current thread's RunLoop. + // Since Operations typically run on a background thread, that would mean the background + // thread's RunLoop. However, the OS can spin down background threads if there's no work + // being done, so we run the risk of the timer's RunLoop being deallocated before it's + // fired. + // + // To ensure the timer's thread sticks around, we schedule it while on the main RunLoop. + self.retryTimer = [NSTimer weakScheduledTimerWithTimeInterval:self.retryInterval + target:self + selector:@selector(runAnyQueuedRetry) + userInfo:nil + repeats:NO]; }); }