From 2d6b9a7c81a82b16ba4d0296c5d01b5f19a14c60 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 27 Feb 2018 11:06:05 -0500 Subject: [PATCH] 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) {