From 9db94095618aaf47e22d1e43d4c3aa0e5a762b3e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 26 Feb 2018 13:49:30 -0500 Subject: [PATCH 1/3] Share background tasks. --- Signal/src/call/CallService.swift | 4 +- SignalMessaging/environment/Release.m | 4 +- SignalServiceKit/src/Util/OWSBackgroundTask.h | 17 +- SignalServiceKit/src/Util/OWSBackgroundTask.m | 327 ++++++++++++++++-- 4 files changed, 316 insertions(+), 36 deletions(-) diff --git a/Signal/src/call/CallService.swift b/Signal/src/call/CallService.swift index 6320e8dec..4efe59ca8 100644 --- a/Signal/src/call/CallService.swift +++ b/Signal/src/call/CallService.swift @@ -557,7 +557,7 @@ protocol CallServiceObserver: class { self.call = newCall - var backgroundTask = OWSBackgroundTask(label:"\(#function)", completionBlock: { [weak self] status in + var backgroundTask: OWSBackgroundTask? = OWSBackgroundTask(label: "\(#function)", completionBlock: { [weak self] status in AssertIsOnMainThread() guard status == .expired else { @@ -1414,7 +1414,7 @@ protocol CallServiceObserver: class { public func handleFailedCall(failedCall: SignalCall?, error: CallError) { AssertIsOnMainThread() - if case CallError.assertionError(description:let description) = error { + if case CallError.assertionError(description: let description) = error { owsFail(description) } diff --git a/SignalMessaging/environment/Release.m b/SignalMessaging/environment/Release.m index 671130afc..c31eb4d0c 100644 --- a/SignalMessaging/environment/Release.m +++ b/SignalMessaging/environment/Release.m @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // #import "Release.h" @@ -7,6 +7,7 @@ #import "NotificationsManager.h" #import "OWSContactsManager.h" #import +#import #import #import @@ -18,6 +19,7 @@ static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ // Order matters here. + [OWSBackgroundTaskManager sharedManager]; TSStorageManager *storageManager = [TSStorageManager sharedManager]; TSNetworkManager *networkManager = [TSNetworkManager sharedManager]; OWSContactsManager *contactsManager = [OWSContactsManager new]; diff --git a/SignalServiceKit/src/Util/OWSBackgroundTask.h b/SignalServiceKit/src/Util/OWSBackgroundTask.h index 7b09ade5f..44bf324b5 100644 --- a/SignalServiceKit/src/Util/OWSBackgroundTask.h +++ b/SignalServiceKit/src/Util/OWSBackgroundTask.h @@ -1,7 +1,9 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // +NS_ASSUME_NONNULL_BEGIN + typedef NS_ENUM(NSUInteger, BackgroundTaskState) { BackgroundTaskState_Success, BackgroundTaskState_CouldNotStart, @@ -10,6 +12,17 @@ typedef NS_ENUM(NSUInteger, BackgroundTaskState) { typedef void (^BackgroundTaskCompletionBlock)(BackgroundTaskState backgroundTaskState); +// This class can be safely accessed and used from any thread. +@interface OWSBackgroundTaskManager : NSObject + +- (instancetype)init NS_UNAVAILABLE; + ++ (instancetype)sharedManager; + +@end + +#pragma mark - + // This class makes it easier and safer to use background tasks. // // * Uses RAII (Resource Acquisition Is Initialization) pattern. @@ -41,3 +54,5 @@ typedef void (^BackgroundTaskCompletionBlock)(BackgroundTaskState backgroundTask completionBlock:(BackgroundTaskCompletionBlock)completionBlock; @end + +NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Util/OWSBackgroundTask.m b/SignalServiceKit/src/Util/OWSBackgroundTask.m index f571aec31..fd9bc9d00 100644 --- a/SignalServiceKit/src/Util/OWSBackgroundTask.m +++ b/SignalServiceKit/src/Util/OWSBackgroundTask.m @@ -4,14 +4,290 @@ #import "OWSBackgroundTask.h" #import "AppContext.h" +#import "NSTimer+OWS.h" #import "Threading.h" +NS_ASSUME_NONNULL_BEGIN + +typedef void (^BackgroundTaskExpirationBlock)(void); +typedef NSNumber *OWSTaskId; + +// This class can be safely accessed and used from any thread. +@interface OWSBackgroundTaskManager () + +// This property should only be accessed while synchronized on this instance. +@property (nonatomic) UIBackgroundTaskIdentifier backgroundTaskId; + +// This property should only be accessed while synchronized on this instance. +@property (nonatomic) NSMutableDictionary *expirationMap; + +// This property should only be accessed while synchronized on this instance. +@property (nonatomic) unsigned long long idCounter; + +// Note that this flag is set a little early in "will resign active". +// +// This property should only be accessed while synchronized on this instance. +@property (nonatomic) BOOL isAppActive; + +// We use this timer to provide continuity and reduce churn, +// so that if one OWSBackgroundTask ends right before another +// begins, we use a single uninterrupted background that +// spans their lifetimes. +// +// This property should only be accessed while synchronized on this instance. +@property (nonatomic, nullable) NSTimer *continuityTimer; + +@end + +#pragma mark - + +@implementation OWSBackgroundTaskManager + ++ (instancetype)sharedManager +{ + static OWSBackgroundTaskManager *sharedMyManager = nil; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + sharedMyManager = [[self alloc] initDefault]; + }); + return sharedMyManager; +} + +- (instancetype)initDefault +{ + OWSAssertIsOnMainThread(); + + self = [super init]; + + if (!self) { + return self; + } + + self.backgroundTaskId = UIBackgroundTaskInvalid; + self.expirationMap = [NSMutableDictionary new]; + self.idCounter = 0; + self.isAppActive = CurrentAppContext().isMainAppAndActive; + + OWSSingletonAssert(); + + [self observeNotifications]; + + return self; +} + +- (void)dealloc +{ + [[NSNotificationCenter defaultCenter] removeObserver:self]; +} + +- (void)observeNotifications +{ + if (!CurrentAppContext().isMainApp) { + return; + } + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(applicationDidBecomeActive:) + name:OWSApplicationDidBecomeActiveNotification + object:nil]; + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(applicationWillResignActive:) + name:OWSApplicationWillResignActiveNotification + object:nil]; +} + +- (void)applicationDidBecomeActive:(UIApplication *)application +{ + OWSAssertIsOnMainThread(); + + @synchronized(self) + { + self.isAppActive = YES; + + [self ensureBackgroundTaskState]; + } +} + +- (void)applicationWillResignActive:(UIApplication *)application +{ + OWSAssertIsOnMainThread(); + + @synchronized(self) + { + self.isAppActive = NO; + + [self ensureBackgroundTaskState]; + } +} + +// Returns nil if adding this task _should have_ started a +// background task, but the background task couldn't be begun. +// In that case expirationBlock will not be called. +- (nullable OWSTaskId)addTask:(BackgroundTaskExpirationBlock)expirationBlock +{ + OWSAssert(expirationBlock); + + OWSTaskId _Nullable taskId; + + @synchronized(self) + { + self.idCounter = self.idCounter + 1; + taskId = @(self.idCounter); + self.expirationMap[taskId] = expirationBlock; + + if (![self ensureBackgroundTaskState]) { + [self.expirationMap removeObjectForKey:taskId]; + return nil; + } + + [self.continuityTimer invalidate]; + self.continuityTimer = nil; + + return taskId; + } +} + +- (void)removeTask:(OWSTaskId)taskId +{ + OWSAssert(taskId); + + @synchronized(self) + { + OWSAssert(self.expirationMap[taskId] != nil); + + [self.expirationMap removeObjectForKey:taskId]; + + // Keep the background task active (if necessary) for an + // extra fraction of a second to provide continuity between + // tasks. + [self.continuityTimer invalidate]; + self.continuityTimer = [NSTimer weakScheduledTimerWithTimeInterval:0.25f + target:self + selector:@selector(timerDidFire) + userInfo:nil + repeats:NO]; + + [self ensureBackgroundTaskState]; + } +} + +// Begins or end a background task if necessary. +- (BOOL)ensureBackgroundTaskState +{ + if (!CurrentAppContext().isMainApp) { + // We can't create background tasks in the SAE, but pretend that we succeeded. + return YES; + } + + @synchronized(self) + { + // We only want to have a background task if we are: + // a) "not active" AND + // b1) there is more than one active instance of OWSBackgroundTask. + // b2) or there _was_ an active instance recently. + BOOL shouldHaveBackgroundTask = (!self.isAppActive && (self.expirationMap.count > 0 || self.continuityTimer)); + BOOL hasBackgroundTask = self.backgroundTaskId != UIBackgroundTaskInvalid; + + if (shouldHaveBackgroundTask == hasBackgroundTask) { + // Current state is correct. + return YES; + } else if (shouldHaveBackgroundTask) { + DDLogInfo(@"%@ Starting background task.", self.logTag); + return [self startBackgroundTask]; + } else { + // Need to end background task. + DDLogInfo(@"%@ Ending background task.", self.logTag); + UIBackgroundTaskIdentifier backgroundTaskId = self.backgroundTaskId; + self.backgroundTaskId = UIBackgroundTaskInvalid; + [CurrentAppContext() endBackgroundTask:backgroundTaskId]; + return YES; + } + } +} + +// Returns NO if the background task cannot be begun. +- (BOOL)startBackgroundTask +{ + OWSAssert(CurrentAppContext().isMainApp); + + @synchronized(self) + { + OWSAssert(self.backgroundTaskId == UIBackgroundTaskInvalid); + + self.backgroundTaskId = [CurrentAppContext() beginBackgroundTaskWithExpirationHandler:^{ + // Supposedly [UIApplication beginBackgroundTaskWithExpirationHandler]'s handler + // will always be called on the main thread, but in practice we've observed + // otherwise. + // + // See: + // https://developer.apple.com/documentation/uikit/uiapplication/1623031-beginbackgroundtaskwithexpiratio) + OWSAssert([NSThread isMainThread]); + + [self backgroundTaskExpired]; + }]; + + // If a background task could not be begun, call the completion block. + if (self.backgroundTaskId == UIBackgroundTaskInvalid) { + DDLogError(@"%@ background task could not be started.", self.logTag); + + return NO; + } + return YES; + } +} + +- (void)backgroundTaskExpired +{ + UIBackgroundTaskIdentifier backgroundTaskId; + NSDictionary *expirationMap; + + @synchronized(self) + { + backgroundTaskId = self.backgroundTaskId; + self.backgroundTaskId = UIBackgroundTaskInvalid; + + expirationMap = [self.expirationMap copy]; + [self.expirationMap removeAllObjects]; + } + + // It'd be nice to do this work synchronously, but it seems unsafe to + // depend on all expiration blocks being cheap. + // + // OWSBackgroundTask's API guarantees that completionBlock will always + // be called on the main thread, so facilitate that by dispatching async + // to main queue here. That way we can ensure that we don't end the + // background task until all of the completion blocks have completed. + dispatch_async(dispatch_get_main_queue(), ^{ + for (BackgroundTaskExpirationBlock expirationBlock in expirationMap) { + expirationBlock(); + } + if (backgroundTaskId != UIBackgroundTaskInvalid) { + // Apparently we need to "end" even expired background tasks. + [CurrentAppContext() endBackgroundTask:backgroundTaskId]; + } + }); +} + +- (void)timerDidFire +{ + @synchronized(self) + { + [self.continuityTimer invalidate]; + self.continuityTimer = nil; + + [self ensureBackgroundTaskState]; + } +} + +@end + +#pragma mark - + @interface OWSBackgroundTask () @property (nonatomic, readonly) NSString *label; // This property should only be accessed while synchronized on this instance. -@property (nonatomic) UIBackgroundTaskIdentifier backgroundTaskId; +@property (nonatomic, nullable) OWSTaskId taskId; // This property should only be accessed while synchronized on this instance. @property (nonatomic, nullable) BackgroundTaskCompletionBlock completionBlock; @@ -76,24 +352,14 @@ - (void)startBackgroundTask { - // beginBackgroundTaskWithExpirationHandler must be called on the main thread. __weak typeof(self) weakSelf = self; - self.backgroundTaskId = [CurrentAppContext() beginBackgroundTaskWithExpirationHandler:^{ - // Supposedly [UIApplication beginBackgroundTaskWithExpirationHandler]'s handler - // will always be called on the main thread, but in practice we've observed - // otherwise. We use DispatchSyncMainThreadSafe() (note the sync) to ensure that - // this work is done on the main thread. - // - // See: https://developer.apple.com/documentation/uikit/uiapplication/1623031-beginbackgroundtaskwithexpiratio) - // - // Note the usage of OWSCAssert() to avoid capturing a reference to self. - OWSCAssert([NSThread isMainThread]); - - DispatchSyncMainThreadSafe(^{ + self.taskId = [OWSBackgroundTaskManager.sharedManager addTask:^{ + DispatchMainThreadSafe(^{ OWSBackgroundTask *strongSelf = weakSelf; if (!strongSelf) { return; } + DDLogVerbose(@"%@ task expired", strongSelf.logTag); // Make a local copy of completionBlock to ensure that it is called // exactly once. @@ -101,11 +367,11 @@ @synchronized(strongSelf) { - if (strongSelf.backgroundTaskId == UIBackgroundTaskInvalid) { + if (!strongSelf.taskId) { return; } DDLogInfo(@"%@ %@ background task expired.", strongSelf.logTag, strongSelf.label); - strongSelf.backgroundTaskId = UIBackgroundTaskInvalid; + strongSelf.taskId = nil; completionBlock = strongSelf.completionBlock; strongSelf.completionBlock = nil; @@ -118,9 +384,8 @@ }]; // If a background task could not be begun, call the completion block. - if (self.backgroundTaskId == UIBackgroundTaskInvalid) { - - DDLogInfo(@"%@ %@ background task could not be started.", self.logTag, self.label); + if (!self.taskId) { + DDLogError(@"%@ %@ background task could not be started.", self.logTag, self.label); // Make a local copy of completionBlock to ensure that it is called // exactly once. @@ -131,7 +396,9 @@ self.completionBlock = nil; } if (completionBlock) { - completionBlock(BackgroundTaskState_CouldNotStart); + DispatchMainThreadSafe(^{ + completionBlock(BackgroundTaskState_CouldNotStart); + }); } } } @@ -139,32 +406,28 @@ - (void)endBackgroundTask { // Make a local copy of this state, since this method is called by `dealloc`. - UIBackgroundTaskIdentifier backgroundTaskId; BackgroundTaskCompletionBlock _Nullable completionBlock; @synchronized(self) { - backgroundTaskId = self.backgroundTaskId; + if (!self.taskId) { + return; + } + [OWSBackgroundTaskManager.sharedManager removeTask:self.taskId]; + self.taskId = nil; + completionBlock = self.completionBlock; self.completionBlock = nil; } - if (backgroundTaskId == UIBackgroundTaskInvalid) { - OWSAssert(!completionBlock); - return; - } - // endBackgroundTask must be called on the main thread. DispatchMainThreadSafe(^{ - if (completionBlock) { completionBlock(BackgroundTaskState_Success); } - - if (backgroundTaskId != UIBackgroundTaskInvalid) { - [CurrentAppContext() endBackgroundTask:backgroundTaskId]; - } }); } @end + +NS_ASSUME_NONNULL_END From 2d6b9a7c81a82b16ba4d0296c5d01b5f19a14c60 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 27 Feb 2018 11:06:05 -0500 Subject: [PATCH 2/3] Respond to CR. --- SignalMessaging/environment/Release.m | 2 +- SignalServiceKit/src/Util/OWSBackgroundTask.h | 2 + SignalServiceKit/src/Util/OWSBackgroundTask.m | 40 ++++++++++--------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/SignalMessaging/environment/Release.m b/SignalMessaging/environment/Release.m index c31eb4d0c..5778a473f 100644 --- a/SignalMessaging/environment/Release.m +++ b/SignalMessaging/environment/Release.m @@ -19,7 +19,7 @@ static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ // Order matters here. - [OWSBackgroundTaskManager sharedManager]; + [[OWSBackgroundTaskManager sharedManager] observeNotifications]; TSStorageManager *storageManager = [TSStorageManager sharedManager]; TSNetworkManager *networkManager = [TSNetworkManager sharedManager]; OWSContactsManager *contactsManager = [OWSContactsManager new]; diff --git a/SignalServiceKit/src/Util/OWSBackgroundTask.h b/SignalServiceKit/src/Util/OWSBackgroundTask.h index 44bf324b5..2f9bb0be9 100644 --- a/SignalServiceKit/src/Util/OWSBackgroundTask.h +++ b/SignalServiceKit/src/Util/OWSBackgroundTask.h @@ -19,6 +19,8 @@ typedef void (^BackgroundTaskCompletionBlock)(BackgroundTaskState backgroundTask + (instancetype)sharedManager; +- (void)observeNotifications; + @end #pragma mark - diff --git a/SignalServiceKit/src/Util/OWSBackgroundTask.m b/SignalServiceKit/src/Util/OWSBackgroundTask.m index fd9bc9d00..d1829d88a 100644 --- a/SignalServiceKit/src/Util/OWSBackgroundTask.m +++ b/SignalServiceKit/src/Util/OWSBackgroundTask.m @@ -70,8 +70,6 @@ typedef NSNumber *OWSTaskId; OWSSingletonAssert(); - [self observeNotifications]; - return self; } @@ -119,10 +117,14 @@ typedef NSNumber *OWSTaskId; } } +// This method registers a new task with this manager. We only bother +// requesting a background task from iOS if the app is inactive (or about +// to become inactive), so this will often not start a background task. +// // Returns nil if adding this task _should have_ started a // background task, but the background task couldn't be begun. // In that case expirationBlock will not be called. -- (nullable OWSTaskId)addTask:(BackgroundTaskExpirationBlock)expirationBlock +- (nullable OWSTaskId)addTaskWithExpirationBlock:(BackgroundTaskExpirationBlock)expirationBlock { OWSAssert(expirationBlock); @@ -156,9 +158,12 @@ typedef NSNumber *OWSTaskId; [self.expirationMap removeObjectForKey:taskId]; - // Keep the background task active (if necessary) for an - // extra fraction of a second to provide continuity between - // tasks. + // This timer will ensure that we keep the background task active (if necessary) + // for an extra fraction of a second to provide continuity between tasks. + // This makes it easier and safer to use background tasks, since most code + // should be able to ensure background tasks by "narrowly" wrapping + // their core logic with a OWSBackgroundTask and not worrying about "hand off" + // between OWSBackgroundTasks. [self.continuityTimer invalidate]; self.continuityTimer = [NSTimer weakScheduledTimerWithTimeInterval:0.25f target:self @@ -182,8 +187,8 @@ typedef NSNumber *OWSTaskId; { // We only want to have a background task if we are: // a) "not active" AND - // b1) there is more than one active instance of OWSBackgroundTask. - // b2) or there _was_ an active instance recently. + // b1) there is one or more active instance of OWSBackgroundTask OR... + // b2) ...there _was_ an active instance recently. BOOL shouldHaveBackgroundTask = (!self.isAppActive && (self.expirationMap.count > 0 || self.continuityTimer)); BOOL hasBackgroundTask = self.backgroundTaskId != UIBackgroundTaskInvalid; @@ -225,7 +230,7 @@ typedef NSNumber *OWSTaskId; [self backgroundTaskExpired]; }]; - // If a background task could not be begun, call the completion block. + // If the background task could not begin, return NO to indicate that. if (self.backgroundTaskId == UIBackgroundTaskInvalid) { DDLogError(@"%@ background task could not be started.", self.logTag); @@ -249,14 +254,13 @@ typedef NSNumber *OWSTaskId; [self.expirationMap removeAllObjects]; } - // It'd be nice to do this work synchronously, but it seems unsafe to - // depend on all expiration blocks being cheap. - // - // OWSBackgroundTask's API guarantees that completionBlock will always - // be called on the main thread, so facilitate that by dispatching async - // to main queue here. That way we can ensure that we don't end the - // background task until all of the completion blocks have completed. - dispatch_async(dispatch_get_main_queue(), ^{ + // Supposedly [UIApplication beginBackgroundTaskWithExpirationHandler]'s handler + // will always be called on the main thread, but in practice we've observed + // otherwise. OWSBackgroundTask's API guarantees that completionBlock will + // always be called on the main thread, so we use DispatchSyncMainThreadSafe() + // to ensure that. We thereby ensure that we don't end the background task + // until all of the completion blocks have completed. + DispatchSyncMainThreadSafe(^{ for (BackgroundTaskExpirationBlock expirationBlock in expirationMap) { expirationBlock(); } @@ -353,7 +357,7 @@ typedef NSNumber *OWSTaskId; - (void)startBackgroundTask { __weak typeof(self) weakSelf = self; - self.taskId = [OWSBackgroundTaskManager.sharedManager addTask:^{ + self.taskId = [OWSBackgroundTaskManager.sharedManager addTaskWithExpirationBlock:^{ DispatchMainThreadSafe(^{ OWSBackgroundTask *strongSelf = weakSelf; if (!strongSelf) { From 4f55079a79b844adc9c72eb86ea07a0a5c985215 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 27 Feb 2018 11:14:25 -0500 Subject: [PATCH 3/3] Respond to CR. --- SignalMessaging/environment/AppSetup.m | 4 ++++ SignalMessaging/environment/Release.m | 2 -- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/SignalMessaging/environment/AppSetup.m b/SignalMessaging/environment/AppSetup.m index 0dfa5da28..48f663d83 100644 --- a/SignalMessaging/environment/AppSetup.m +++ b/SignalMessaging/environment/AppSetup.m @@ -10,6 +10,7 @@ #import #import #import +#import #import #import @@ -25,6 +26,9 @@ NS_ASSUME_NONNULL_BEGIN static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ + // Order matters here. + [[OWSBackgroundTaskManager sharedManager] observeNotifications]; + [Environment setCurrent:[Release releaseEnvironment]]; id callMessageHandler = callMessageHandlerBlock(); diff --git a/SignalMessaging/environment/Release.m b/SignalMessaging/environment/Release.m index 5778a473f..6b8c9b630 100644 --- a/SignalMessaging/environment/Release.m +++ b/SignalMessaging/environment/Release.m @@ -7,7 +7,6 @@ #import "NotificationsManager.h" #import "OWSContactsManager.h" #import -#import #import #import @@ -19,7 +18,6 @@ static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ // Order matters here. - [[OWSBackgroundTaskManager sharedManager] observeNotifications]; TSStorageManager *storageManager = [TSStorageManager sharedManager]; TSNetworkManager *networkManager = [TSNetworkManager sharedManager]; OWSContactsManager *contactsManager = [OWSContactsManager new];