From 8bd21fd023ad1b396d028fc1c17f268e53566bc5 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 28 Nov 2018 17:01:26 -0500 Subject: [PATCH] Respond to CR. --- .../OWSBackupSettingsViewController.m | 2 +- .../ViewControllers/DebugUI/DebugUIBackup.m | 2 +- Signal/src/util/Backup/OWSBackup.h | 2 +- Signal/src/util/Backup/OWSBackup.m | 12 +- Signal/src/util/Backup/OWSBackupAPI.swift | 114 +++++++++--------- Signal/src/util/Backup/OWSBackupExportJob.h | 2 +- Signal/src/util/Backup/OWSBackupExportJob.m | 70 ++++------- Signal/src/util/Backup/OWSBackupImportJob.m | 2 +- .../Util/Promise+retainUntilComplete.swift | 16 +-- 9 files changed, 97 insertions(+), 125 deletions(-) diff --git a/Signal/src/ViewControllers/AppSettings/OWSBackupSettingsViewController.m b/Signal/src/ViewControllers/AppSettings/OWSBackupSettingsViewController.m index f02cf6f3a..0dae62597 100644 --- a/Signal/src/ViewControllers/AppSettings/OWSBackupSettingsViewController.m +++ b/Signal/src/ViewControllers/AppSettings/OWSBackupSettingsViewController.m @@ -72,7 +72,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)updateICloudStatus { __weak OWSBackupSettingsViewController *weakSelf = self; - [[self.backup checkCloudKitAccess] + [[self.backup ensureCloudKitAccess] .then(^{ OWSAssertIsOnMainThread(); diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m b/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m index 56d220b49..880ec8bf8 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m +++ b/Signal/src/ViewControllers/DebugUI/DebugUIBackup.m @@ -88,7 +88,7 @@ NS_ASSUME_NONNULL_BEGIN OWSAssertDebug(success); NSString *recipientId = self.tsAccountManager.localNumber; - [[self.backup checkCloudKitAccess].then(^{ + [[self.backup ensureCloudKitAccess].then(^{ return [OWSBackupAPI saveTestFileToCloudObjcWithRecipientId:recipientId fileUrl:[NSURL fileURLWithPath:filePath]]; }) retainUntilComplete]; diff --git a/Signal/src/util/Backup/OWSBackup.h b/Signal/src/util/Backup/OWSBackup.h index d1515f824..38211f101 100644 --- a/Signal/src/util/Backup/OWSBackup.h +++ b/Signal/src/util/Backup/OWSBackup.h @@ -74,7 +74,7 @@ NSError *OWSBackupErrorWithDescription(NSString *description); - (void)allRecipientIdsWithManifestsInCloud:(OWSBackupStringListBlock)success failure:(OWSBackupErrorBlock)failure; -- (AnyPromise *)checkCloudKitAccess; +- (AnyPromise *)ensureCloudKitAccess; - (AnyPromise *)checkCanExportBackup; diff --git a/Signal/src/util/Backup/OWSBackup.m b/Signal/src/util/Backup/OWSBackup.m index 055115f59..6e37a79f6 100644 --- a/Signal/src/util/Backup/OWSBackup.m +++ b/Signal/src/util/Backup/OWSBackup.m @@ -217,7 +217,7 @@ NSError *OWSBackupErrorWithDescription(NSString *description) _backupExportState = OWSBackupState_InProgress; self.backupExportJob = [[OWSBackupExportJob alloc] initWithDelegate:self recipientId:recipientId]; - [self.backupExportJob startAsync]; + [self.backupExportJob start]; [self postDidChangeNotification]; } @@ -369,7 +369,7 @@ NSError *OWSBackupErrorWithDescription(NSString *description) self.backupExportJob = nil; } else if (self.shouldHaveBackupExport && !self.backupExportJob) { self.backupExportJob = [[OWSBackupExportJob alloc] initWithDelegate:self recipientId:recipientId]; - [self.backupExportJob startAsync]; + [self.backupExportJob start]; } // Update the state flag. @@ -423,10 +423,10 @@ NSError *OWSBackupErrorWithDescription(NSString *description) - (AnyPromise *)checkCanExportBackup { - return [self checkCloudKitAccess]; + return [self ensureCloudKitAccess]; } -- (AnyPromise *)checkCloudKitAccess +- (AnyPromise *)ensureCloudKitAccess { OWSAssertIsOnMainThread(); @@ -452,7 +452,7 @@ NSError *OWSBackupErrorWithDescription(NSString *description) return failWithUnexpectedError(); } - return [[OWSBackupAPI checkCloudKitAccessObjc] retainUntilComplete]; + return [OWSBackupAPI ensureCloudKitAccessObjc]; } - (void)checkCanImportBackup:(OWSBackupBoolBlock)success failure:(OWSBackupErrorBlock)failure @@ -484,7 +484,7 @@ NSError *OWSBackupErrorWithDescription(NSString *description) return failWithUnexpectedError(); } - [[OWSBackupAPI checkCloudKitAccessObjc] + [[OWSBackupAPI ensureCloudKitAccessObjc] .thenInBackground(^{ return [OWSBackupAPI checkForManifestInCloudObjcWithRecipientId:recipientId]; }) diff --git a/Signal/src/util/Backup/OWSBackupAPI.swift b/Signal/src/util/Backup/OWSBackupAPI.swift index ce660b6c8..93ecdb806 100644 --- a/Signal/src/util/Backup/OWSBackupAPI.swift +++ b/Signal/src/util/Backup/OWSBackupAPI.swift @@ -202,63 +202,61 @@ import PromiseKit } public class func saveRecordToCloud(record: CKRecord) -> Promise { - let (promise, resolver) = Promise.pending() - saveRecordToCloud(record: record, - remainingRetries: maxRetries, - success: { (recordName) in - resolver.fulfill(recordName) - }, - failure: { (error) in - resolver.reject(error) - }) - return promise.retainUntilComplete() + return saveRecordToCloud(record: record, + remainingRetries: maxRetries) } private class func saveRecordToCloud(record: CKRecord, - remainingRetries: Int, - success: @escaping (String) -> Void, - failure: @escaping (Error) -> Void) { - - let saveOperation = CKModifyRecordsOperation(recordsToSave: [record ], recordIDsToDelete: nil) - saveOperation.modifyRecordsCompletionBlock = { (records, recordIds, error) in - - let outcome = outcomeForCloudKitError(error: error, - remainingRetries: remainingRetries, - label: "Save Record") - switch outcome { - case .success: - let recordName = record.recordID.recordName - success(recordName) - case .failureDoNotRetry(let outcomeError): - failure(outcomeError) - case .failureRetryAfterDelay(let retryDelay): - DispatchQueue.global().asyncAfter(deadline: DispatchTime.now() + retryDelay, execute: { - saveRecordToCloud(record: record, - remainingRetries: remainingRetries - 1, - success: success, - failure: failure) - }) - case .failureRetryWithoutDelay: - DispatchQueue.global().async { - saveRecordToCloud(record: record, - remainingRetries: remainingRetries - 1, - success: success, - failure: failure) + remainingRetries: Int) -> Promise { + + return Promise { resolver in + let saveOperation = CKModifyRecordsOperation(recordsToSave: [record ], recordIDsToDelete: nil) + saveOperation.modifyRecordsCompletionBlock = { (records, recordIds, error) in + + let outcome = outcomeForCloudKitError(error: error, + remainingRetries: remainingRetries, + label: "Save Record") + switch outcome { + case .success: + let recordName = record.recordID.recordName + resolver.fulfill(recordName) + case .failureDoNotRetry(let outcomeError): + resolver.reject(outcomeError) + case .failureRetryAfterDelay(let retryDelay): + DispatchQueue.global().asyncAfter(deadline: DispatchTime.now() + retryDelay, execute: { + saveRecordToCloud(record: record, + remainingRetries: remainingRetries - 1) + .done { (recordName) in + resolver.fulfill(recordName) + }.catch { (error) in + resolver.reject(error) + }.retainUntilComplete() + }) + case .failureRetryWithoutDelay: + DispatchQueue.global().async { + saveRecordToCloud(record: record, + remainingRetries: remainingRetries - 1) + .done { (recordName) in + resolver.fulfill(recordName) + }.catch { (error) in + resolver.reject(error) + }.retainUntilComplete() + } + case .unknownItem: + owsFailDebug("unexpected CloudKit response.") + resolver.reject(invalidServiceResponseError()) } - case .unknownItem: - owsFailDebug("unexpected CloudKit response.") - failure(invalidServiceResponseError()) } - } - saveOperation.isAtomic = false + saveOperation.isAtomic = false - // These APIs are only available in iOS 9.3 and later. - if #available(iOS 9.3, *) { - saveOperation.isLongLived = true - saveOperation.qualityOfService = .background - } + // These APIs are only available in iOS 9.3 and later. + if #available(iOS 9.3, *) { + saveOperation.isLongLived = true + saveOperation.qualityOfService = .background + } - database().add(saveOperation) + database().add(saveOperation) + } } // Compare: @@ -422,7 +420,7 @@ import PromiseKit resolver.fulfill(record) }.catch { (error) in resolver.reject(error) - } + }.retainUntilComplete() }) case .failureRetryWithoutDelay: DispatchQueue.global().async { @@ -432,7 +430,7 @@ import PromiseKit resolver.fulfill(record) }.catch { (error) in resolver.reject(error) - } + }.retainUntilComplete() } case .unknownItem: // Record not found. @@ -440,7 +438,7 @@ import PromiseKit } } database().add(fetchOperation) - return promise.retainUntilComplete() + return promise } @objc @@ -701,13 +699,13 @@ import PromiseKit } @objc - public class func checkCloudKitAccessObjc() -> AnyPromise { - return AnyPromise(checkCloudKitAccess()) + public class func ensureCloudKitAccessObjc() -> AnyPromise { + return AnyPromise(ensureCloudKitAccess()) } - public class func checkCloudKitAccess() -> Promise { + public class func ensureCloudKitAccess() -> Promise { let (promise, resolver) = Promise.pending() - CKContainer.default().accountStatus(completionHandler: { (accountStatus, error) in + CKContainer.default().accountStatus { (accountStatus, error) in if let error = error { Logger.error("Unknown error: \(String(describing: error)).") resolver.reject(error) @@ -727,7 +725,7 @@ import PromiseKit Logger.verbose("CloudKit access okay.") resolver.fulfill(()) } - }) + } return promise } diff --git a/Signal/src/util/Backup/OWSBackupExportJob.h b/Signal/src/util/Backup/OWSBackupExportJob.h index f72f742f0..8fa9b475e 100644 --- a/Signal/src/util/Backup/OWSBackupExportJob.h +++ b/Signal/src/util/Backup/OWSBackupExportJob.h @@ -8,7 +8,7 @@ NS_ASSUME_NONNULL_BEGIN @interface OWSBackupExportJob : OWSBackupJob -- (void)startAsync; +- (void)start; @end diff --git a/Signal/src/util/Backup/OWSBackupExportJob.m b/Signal/src/util/Backup/OWSBackupExportJob.m index 76bcff8c0..6ff6ed111 100644 --- a/Signal/src/util/Backup/OWSBackupExportJob.m +++ b/Signal/src/util/Backup/OWSBackupExportJob.m @@ -337,7 +337,7 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - -- (void)startAsync +- (void)start { OWSAssertIsOnMainThread(); @@ -347,24 +347,14 @@ NS_ASSUME_NONNULL_BEGIN [self updateProgressWithDescription:nil progress:nil]; - [[self.backup checkCloudKitAccess] + [[self.backup ensureCloudKitAccess] .thenInBackground(^{ - [self start]; - }) - .catch(^(NSError *error) { - [self failWithErrorDescription: - NSLocalizedString(@"BACKUP_EXPORT_ERROR_COULD_NOT_EXPORT", - @"Error indicating the backup export could not export the user's data.")]; - }) retainUntilComplete]; -} - -- (void)start -{ - [self updateProgressWithDescription:NSLocalizedString(@"BACKUP_EXPORT_PHASE_CONFIGURATION", - @"Indicates that the backup export is being configured.") - progress:nil]; + [self updateProgressWithDescription:NSLocalizedString(@"BACKUP_EXPORT_PHASE_CONFIGURATION", + @"Indicates that the backup export is being configured.") + progress:nil]; - [[self configureExport] + return [self configureExport]; + }) .thenInBackground(^{ return [self fetchAllRecords]; }) @@ -415,7 +405,7 @@ NS_ASSUME_NONNULL_BEGIN // // We use an arbitrary request that requires authentication // to verify our account state. - AnyPromise *promise = [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { + return [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { TSRequest *currentSignedPreKey = [OWSRequestFactory currentSignedPreKeyRequest]; [[TSNetworkManager sharedManager] makeRequest:currentSignedPreKey success:^(NSURLSessionDataTask *task, NSDictionary *responseObject) { @@ -427,8 +417,6 @@ NS_ASSUME_NONNULL_BEGIN resolve(error); }]; }]; - [promise retainUntilComplete]; - return promise; } - (AnyPromise *)fetchAllRecords @@ -439,7 +427,7 @@ NS_ASSUME_NONNULL_BEGIN return [AnyPromise promiseWithValue:OWSBackupErrorWithDescription(@"Backup export no longer active.")]; } - AnyPromise *promise = [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { + return [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { [OWSBackupAPI fetchAllRecordNamesWithRecipientId:self.recipientId success:^(NSArray *recordNames) { if (self.isComplete) { @@ -452,8 +440,6 @@ NS_ASSUME_NONNULL_BEGIN resolve(error); }]; }]; - [promise retainUntilComplete]; - return promise; } - (AnyPromise *)exportDatabase @@ -750,25 +736,23 @@ NS_ASSUME_NONNULL_BEGIN for (OWSBackupExportItem *item in self.unsavedDatabaseItems) { OWSAssertDebug(item.encryptedItem.filePath.length > 0); - promise = promise.thenInBackground(^{ - return [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { - if (self.isComplete) { - return resolve(OWSBackupErrorWithDescription(@"Backup export no longer active.")); - } - resolve(@(1)); - }] - .thenInBackground(^{ - return [OWSBackupAPI - saveEphemeralDatabaseFileToCloudObjcWithRecipientId:self.recipientId - fileUrl:[NSURL fileURLWithPath:item.encryptedItem - .filePath]]; - }) - .thenInBackground(^(NSString *recordName) { - item.recordName = recordName; - [self.savedDatabaseItems addObject:item]; - return [AnyPromise promiseWithValue:@(1)]; - }); - }); + promise + = promise + .thenInBackground(^{ + if (self.isComplete) { + return [AnyPromise + promiseWithValue:OWSBackupErrorWithDescription(@"Backup export no longer active.")]; + } + + return [OWSBackupAPI + saveEphemeralDatabaseFileToCloudObjcWithRecipientId:self.recipientId + fileUrl:[NSURL fileURLWithPath:item.encryptedItem + .filePath]]; + }) + .thenInBackground(^(NSString *recordName) { + item.recordName = recordName; + [self.savedDatabaseItems addObject:item]; + }); } [self.unsavedDatabaseItems removeAllObjects]; return promise; @@ -880,7 +864,6 @@ NS_ASSUME_NONNULL_BEGIN OWSLogVerbose( @"saved attachment: %@ as %@", attachmentExport.attachmentFilePath, attachmentExport.relativeFilePath); - return [AnyPromise promiseWithValue:@(1)]; }) .catchInBackground(^{ if (![attachmentExport cleanUp]) { @@ -914,7 +897,6 @@ NS_ASSUME_NONNULL_BEGIN self.manifestItem = exportItem; // All files have been saved to the cloud. - return [AnyPromise promiseWithValue:@(1)]; }); } diff --git a/Signal/src/util/Backup/OWSBackupImportJob.m b/Signal/src/util/Backup/OWSBackupImportJob.m index 7343cbac0..736fbab31 100644 --- a/Signal/src/util/Backup/OWSBackupImportJob.m +++ b/Signal/src/util/Backup/OWSBackupImportJob.m @@ -77,7 +77,7 @@ NSString *const kOWSBackup_ImportDatabaseKeySpec = @"kOWSBackup_ImportDatabaseKe [self updateProgressWithDescription:nil progress:nil]; __weak OWSBackupImportJob *weakSelf = self; - [[self.backup checkCloudKitAccess] + [[self.backup ensureCloudKitAccess] .thenInBackground(^{ [weakSelf start]; }) diff --git a/SignalServiceKit/src/Util/Promise+retainUntilComplete.swift b/SignalServiceKit/src/Util/Promise+retainUntilComplete.swift index 93593efe1..34a9f60c0 100644 --- a/SignalServiceKit/src/Util/Promise+retainUntilComplete.swift +++ b/SignalServiceKit/src/Util/Promise+retainUntilComplete.swift @@ -11,14 +11,12 @@ public extension AnyPromise { * promise to self retain, until it completes to avoid the risk it's GC'd before completion. */ @objc - @discardableResult - func retainUntilComplete() -> AnyPromise { + func retainUntilComplete() { var retainCycle: AnyPromise? = self _ = self.ensure { assert(retainCycle != nil) retainCycle = nil } - return self } } @@ -27,14 +25,12 @@ public extension PMKFinalizer { * Sometimes there isn't a straight forward candidate to retain a promise, in that case we tell the * promise to self retain, until it completes to avoid the risk it's GC'd before completion. */ - @discardableResult - func retainUntilComplete() -> PMKFinalizer { + func retainUntilComplete() { var retainCycle: PMKFinalizer? = self _ = self.finally { assert(retainCycle != nil) retainCycle = nil } - return self } } @@ -43,14 +39,12 @@ public extension Promise { * Sometimes there isn't a straight forward candidate to retain a promise, in that case we tell the * promise to self retain, until it completes to avoid the risk it's GC'd before completion. */ - @discardableResult - func retainUntilComplete() -> Promise { + func retainUntilComplete() { var retainCycle: Promise? = self _ = self.ensure { assert(retainCycle != nil) retainCycle = nil } - return self } } @@ -59,13 +53,11 @@ public extension Guarantee { * Sometimes there isn't a straight forward candidate to retain a promise, in that case we tell the * promise to self retain, until it completes to avoid the risk it's GC'd before completion. */ - @discardableResult - func retainUntilComplete() -> Guarantee { + func retainUntilComplete() { var retainCycle: Guarantee? = self _ = self.done { _ in assert(retainCycle != nil) retainCycle = nil } - return self } }