From f277ae877c4340dba0c356eab84372405bee6b15 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 18 Jul 2018 13:52:44 -0600 Subject: [PATCH 1/8] Clarify OWSOperation behavior (no behavioral changes) --- SignalServiceKit/src/Util/OWSOperation.h | 16 ++++++++++------ SignalServiceKit/src/Util/OWSOperation.m | 1 + 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/SignalServiceKit/src/Util/OWSOperation.h b/SignalServiceKit/src/Util/OWSOperation.h index 102f46631..c86c90394 100644 --- a/SignalServiceKit/src/Util/OWSOperation.h +++ b/SignalServiceKit/src/Util/OWSOperation.h @@ -24,22 +24,26 @@ typedef NS_ENUM(NSInteger, OWSOperationState) { // any of the errors were fatal. Fatal errors trump retryable errors. @interface OWSOperation : NSOperation -@property (nullable) NSError *failingError; +@property (readonly, nullable) NSError *failingError; + +// Defaults to 0, set to greater than 0 in init if you'd like the operation to be retryable. @property NSUInteger remainingRetries; -#pragma mark - Subclass Overrides - -// Called one time only -- (nullable NSError *)checkForPreconditionError; +#pragma mark - Mandatory Subclass Overrides // Called every retry, this is where the bulk of the operation's work should go. - (void)run; +#pragma mark - Optional Subclass Overrides + +// Called one time only +- (nullable NSError *)checkForPreconditionError; + // Called at most one time. - (void)didSucceed; // Called at most one time, once retry is no longer possible. -- (void)didFailWithError:(NSError *)error; +- (void)didFailWithError:(NSError *)error NS_SWIFT_NAME(didFail(error:)); #pragma mark - Success/Error - Do Not Override diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index e9f02949e..7b081f0b3 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -13,6 +13,7 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; @interface OWSOperation () +@property (nullable) NSError *failingError; @property (atomic) OWSOperationState operationState; @property (nonatomic) OWSBackgroundTask *backgroundTask; From b7288b25659109c3f6ea0e3843e460dba50dd356 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 18 Jul 2018 14:46:46 -0600 Subject: [PATCH 2/8] Move contact intersection into batched operation // FREEBIE --- .../src/Contacts/ContactsUpdater.m | 103 ++++----- .../OWSContactDiscoveryOperation.swift | 196 ++++++++++++++++++ .../src/Messages/OWSMessageSender.m | 18 +- .../Network/API/Requests/OWSRequestFactory.h | 2 +- .../Network/API/Requests/OWSRequestFactory.m | 2 +- SignalServiceKit/src/SignalServiceKit.h | 6 + SignalServiceKit/src/Util/OWSOperation.m | 20 +- 7 files changed, 262 insertions(+), 85 deletions(-) create mode 100644 SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift create mode 100644 SignalServiceKit/src/SignalServiceKit.h diff --git a/SignalServiceKit/src/Contacts/ContactsUpdater.m b/SignalServiceKit/src/Contacts/ContactsUpdater.m index dfd0d69c0..8117b5845 100644 --- a/SignalServiceKit/src/Contacts/ContactsUpdater.m +++ b/SignalServiceKit/src/Contacts/ContactsUpdater.m @@ -9,10 +9,18 @@ #import "OWSRequestFactory.h" #import "PhoneNumber.h" #import "TSNetworkManager.h" +#import #import NS_ASSUME_NONNULL_BEGIN +@interface ContactsUpdater () + +@property (nonatomic, readonly) NSOperationQueue *contactIntersectionQueue; + +@end + + @implementation ContactsUpdater + (instancetype)sharedUpdater { @@ -32,6 +40,8 @@ NS_ASSUME_NONNULL_BEGIN return self; } + _contactIntersectionQueue = [NSOperationQueue new]; + OWSSingletonAssert(); return self; @@ -88,75 +98,36 @@ NS_ASSUME_NONNULL_BEGIN - (void)contactIntersectionWithSet:(NSSet *)recipientIdsToLookup success:(void (^)(NSSet *recipients))success - failure:(void (^)(NSError *error))failure { + failure:(void (^)(NSError *error))failure +{ dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - NSMutableDictionary *phoneNumbersByHashes = [NSMutableDictionary new]; - for (NSString *recipientId in recipientIdsToLookup) { - NSString *hash = [Cryptography truncatedSHA1Base64EncodedWithoutPadding:recipientId]; - phoneNumbersByHashes[hash] = recipientId; - } - NSArray *hashes = [phoneNumbersByHashes allKeys]; - - TSRequest *request = [OWSRequestFactory contactsIntersectionRequestWithHashesArray:hashes]; - [[TSNetworkManager sharedManager] makeRequest:request - success:^(NSURLSessionDataTask *task, id responseDict) { - NSMutableSet *registeredRecipientIds = [NSMutableSet new]; - - if ([responseDict isKindOfClass:[NSDictionary class]]) { - NSArray *_Nullable contactsArray = responseDict[@"contacts"]; - if ([contactsArray isKindOfClass:[NSArray class]]) { - for (NSDictionary *contactDict in contactsArray) { - if (![contactDict isKindOfClass:[NSDictionary class]]) { - OWSProdLogAndFail(@"%@ invalid contact dictionary.", self.logTag); - continue; - } - NSString *_Nullable hash = contactDict[@"token"]; - if (hash.length < 1) { - OWSProdLogAndFail(@"%@ contact missing hash.", self.logTag); - continue; - } - NSString *_Nullable recipientId = phoneNumbersByHashes[hash]; - if (recipientId.length < 1) { - OWSProdLogAndFail(@"%@ An intersecting hash wasn't found in the mapping.", self.logTag); - continue; - } - if (![recipientIdsToLookup containsObject:recipientId]) { - OWSProdLogAndFail(@"%@ Intersection response included unexpected recipient.", self.logTag); - continue; - } - [registeredRecipientIds addObject:recipientId]; - } - } - } - - NSMutableSet *recipients = [NSMutableSet new]; - [OWSPrimaryStorage.dbReadWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - for (NSString *recipientId in recipientIdsToLookup) { - if ([registeredRecipientIds containsObject:recipientId]) { - SignalRecipient *recipient = - [SignalRecipient markRecipientAsRegisteredAndGet:recipientId transaction:transaction]; - [recipients addObject:recipient]; - } else { - [SignalRecipient removeUnregisteredRecipient:recipientId transaction:transaction]; - } - } - }]; + OWSContactDiscoveryOperation *operation = + [[OWSContactDiscoveryOperation alloc] initWithRecipientIdsToLookup:recipientIdsToLookup.allObjects]; - success([recipients copy]); - } - failure:^(NSURLSessionDataTask *task, NSError *error) { - if (!IsNSErrorNetworkFailure(error)) { - OWSProdError([OWSAnalyticsEvents contactsErrorContactsIntersectionFailed]); - } + NSArray *operationAndDependencies = [operation.dependencies arrayByAddingObject:operation]; + [self.contactIntersectionQueue addOperations:operationAndDependencies waitUntilFinished:YES]; - NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response; - if (response.statusCode == 413) { - failure(OWSErrorWithCodeDescription( - OWSErrorCodeContactsUpdaterRateLimit, @"Contacts Intersection Rate Limit")); - } else { - failure(error); - } - }]; + if (operation.failingError != nil) { + failure(operation.failingError); + return; + } + + NSSet *registeredRecipientIds = operation.registeredRecipientIds; + + NSMutableSet *recipients = [NSMutableSet new]; + [OWSPrimaryStorage.dbReadWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + for (NSString *recipientId in recipientIdsToLookup) { + if ([registeredRecipientIds containsObject:recipientId]) { + SignalRecipient *recipient = + [SignalRecipient markRecipientAsRegisteredAndGet:recipientId transaction:transaction]; + [recipients addObject:recipient]; + } else { + [SignalRecipient removeUnregisteredRecipient:recipientId transaction:transaction]; + } + } + }]; + + success([recipients copy]); }); } diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift new file mode 100644 index 000000000..58c741d35 --- /dev/null +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -0,0 +1,196 @@ +// +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// + +import Foundation + +extension Array { + func chunked(by chunkSize: Int) -> [[Element]] { + return stride(from: 0, to: self.count, by: chunkSize).map { + Array(self[$0.. + + @objc + required init(recipientIdsToLookup: [String]) { + self.recipientIdsToLookup = recipientIdsToLookup + self.registeredRecipientIds = Set() + + super.init() + + Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") + for batchIds in recipientIdsToLookup.chunked(by: batchSize) { + let batchOperation = OWSContactDiscoveryBatchOperation(recipientIdsToLookup: batchIds) + self.addDependency(batchOperation) + } + } + + // MARK: Mandatory overrides + // Called every retry, this is where the bulk of the operation's work should go. + override func run() { + Logger.debug("\(logTag) in \(#function)") + + for dependency in self.dependencies { + guard let batchOperation = dependency as? OWSContactDiscoveryBatchOperation else { + owsFail("\(self.logTag) in \(#function) unexpected dependency: \(dependency)") + continue + } + + self.registeredRecipientIds.formUnion(batchOperation.registeredRecipientIds) + } + + self.reportSuccess() + } + + // MARK: Optional Overrides + + // Called one time only + override func checkForPreconditionError() -> Error? { + return super.checkForPreconditionError() + } + + // Called at most one time. + override func didSucceed() { + super.didSucceed() + } + + // Called at most one time, once retry is no longer possible. + override func didFail(error: Error) { + super.didFail(error: error) + } +} + +class OWSContactDiscoveryBatchOperation: OWSOperation { + + private let recipientIdsToLookup: [String] + var registeredRecipientIds: Set + + required init(recipientIdsToLookup: [String]) { + self.recipientIdsToLookup = recipientIdsToLookup + self.registeredRecipientIds = Set() + + super.init() + + Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") + } + + private var networkManager: TSNetworkManager { + return TSNetworkManager.shared() + } + + private func parse(response: Any?, phoneNumbersByHashes: [String: String]) throws -> Set { + + guard let responseDict = response as? [String: AnyObject] else { + let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError + responseError.isRetryable = true + + throw responseError + } + + guard let contactDicts = responseDict["contacts"] as? [[String: AnyObject]] else { + let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError + responseError.isRetryable = true + + throw responseError + } + + var registeredRecipientIds: Set = Set() + + for contactDict in contactDicts { + guard let hash = contactDict["token"] as? String, hash.count > 0 else { + owsFail("\(self.logTag) in \(#function) hash was unexpectedly nil") + continue + } + + guard let recipientId = phoneNumbersByHashes[hash], recipientId.count > 0 else { + owsFail("\(self.logTag) in \(#function) recipientId was unexpectedly nil") + continue + } + + guard recipientIdsToLookup.contains(recipientId) else { + owsFail("\(self.logTag) in \(#function) unexpected recipientId") + continue + } + + registeredRecipientIds.insert(recipientId) + } + + return registeredRecipientIds + } + + // MARK: Mandatory overrides + // Called every retry, this is where the bulk of the operation's work should go. + override func run() { + Logger.debug("\(logTag) in \(#function)") + + var phoneNumbersByHashes: [String: String] = [:] + + for recipientId in recipientIdsToLookup { + let hash = Cryptography.truncatedSHA1Base64EncodedWithoutPadding(recipientId) + phoneNumbersByHashes[hash] = recipientId + } + + let hashes: [String] = Array(phoneNumbersByHashes.keys) + + let request = OWSRequestFactory.contactsIntersectionRequest(withHashesArray: hashes) + + self.networkManager.makeRequest(request, + success: { (task, responseDict) in + do { + self.registeredRecipientIds = try self.parse(response: responseDict, phoneNumbersByHashes: phoneNumbersByHashes) + self.reportSuccess() + } catch { + self.reportError(error) + } + }, + failure: { (task, error) in + if (!IsNSErrorNetworkFailure(error)) { + // FIXME not accessible in swift for some reason. +// OWSProdError(OWSAnalyticsEvents.contactsErrorContactsIntersectionFailed) + } + + guard let response = task.response as? HTTPURLResponse else { + let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError + responseError.isRetryable = true + self.reportError(responseError) + return + } + + if (response.statusCode == 413) { + let rateLimitError = OWSErrorWithCodeDescription(OWSErrorCode.contactsUpdaterRateLimit, "Contacts Intersection Rate Limit") + self.reportError(rateLimitError) + } + self.reportError(error) + + }) + } + + // MARK: Optional Overrides + + // Called one time only + override func checkForPreconditionError() -> Error? { + return super.checkForPreconditionError() + } + + // Called at most one time. + override func didSucceed() { + super.didSucceed() + } + + // Called at most one time, once retry is no longer possible. + override func didFail(error: Error) { + super.didFail(error: error) + } +} diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 018977468..24240aba8 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -132,21 +132,9 @@ void AssertIsOnSendingQueue() - (nullable NSError *)checkForPreconditionError { - for (NSOperation *dependency in self.dependencies) { - if (![dependency isKindOfClass:[OWSOperation class]]) { - NSString *errorDescription = - [NSString stringWithFormat:@"%@ unknown dependency: %@", self.logTag, dependency.class]; - NSError *assertionError = OWSErrorMakeAssertionError(errorDescription); - return assertionError; - } - - OWSOperation *upload = (OWSOperation *)dependency; - - // Cannot proceed if dependency failed - surface the dependency's error. - NSError *_Nullable dependencyError = upload.failingError; - if (dependencyError) { - return dependencyError; - } + NSError *_Nullable error = [super checkForPreconditionError]; + if (error) { + return error; } // Sanity check preconditions diff --git a/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.h b/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.h index 9ea9864c1..d6597ed89 100644 --- a/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.h +++ b/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.h @@ -41,7 +41,7 @@ typedef NS_ENUM(NSUInteger, TSVerificationTransport) { TSVerificationTransportVo + (TSRequest *)availablePreKeysCountRequest; -+ (TSRequest *)contactsIntersectionRequestWithHashesArray:(NSArray *)hashes; ++ (TSRequest *)contactsIntersectionRequestWithHashesArray:(NSArray *)hashes; + (TSRequest *)currentSignedPreKeyRequest; diff --git a/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.m b/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.m index 334252fc0..e00460a0a 100644 --- a/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.m +++ b/SignalServiceKit/src/Network/API/Requests/OWSRequestFactory.m @@ -116,7 +116,7 @@ NS_ASSUME_NONNULL_BEGIN return [TSRequest requestWithUrl:[NSURL URLWithString:path] method:@"GET" parameters:@{}]; } -+ (TSRequest *)contactsIntersectionRequestWithHashesArray:(NSArray *)hashes ++ (TSRequest *)contactsIntersectionRequestWithHashesArray:(NSArray *)hashes { OWSAssert(hashes.count > 0); diff --git a/SignalServiceKit/src/SignalServiceKit.h b/SignalServiceKit/src/SignalServiceKit.h new file mode 100644 index 000000000..695adba43 --- /dev/null +++ b/SignalServiceKit/src/SignalServiceKit.h @@ -0,0 +1,6 @@ +// +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// + +// ObjC classes from which Swift classes inherit must be included in this framework header. +#import "OWSOperation.h" diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index 7b081f0b3..d70cbb4c6 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -5,6 +5,7 @@ #import "OWSOperation.h" #import "NSError+MessageSending.h" #import "OWSBackgroundTask.h" +#import "OWSError.h" NS_ASSUME_NONNULL_BEGIN @@ -47,8 +48,23 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; // Called one time only - (nullable NSError *)checkForPreconditionError { - // no-op - // Override in subclass if necessary + for (NSOperation *dependency in self.dependencies) { + if (![dependency isKindOfClass:[OWSOperation class]]) { + NSString *errorDescription = + [NSString stringWithFormat:@"%@ unknown dependency: %@", self.logTag, dependency.class]; + + return OWSErrorMakeAssertionError(errorDescription); + } + + OWSOperation *dependentOperation = (OWSOperation *)dependency; + + // Don't proceed if dependency failed - surface the dependency's error. + NSError *_Nullable dependencyError = dependentOperation.failingError; + if (dependencyError != nil) { + return dependencyError; + } + } + return nil; } From 75248b5dc759486c32995d7f7b858d9a1fbf3f81 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 18 Jul 2018 22:22:27 -0600 Subject: [PATCH 3/8] Stub out feedback operation // FREEBIE --- .../OWSContactDiscoveryOperation.swift | 137 ++++++++++++------ 1 file changed, 92 insertions(+), 45 deletions(-) diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index 58c741d35..6b45e0deb 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -4,20 +4,11 @@ import Foundation -extension Array { - func chunked(by chunkSize: Int) -> [[Element]] { - return stride(from: 0, to: self.count, by: chunkSize).map { - Array(self[$0.. Error? { - return super.checkForPreconditionError() - } - - // Called at most one time. - override func didSucceed() { - super.didSucceed() - } - - // Called at most one time, once retry is no longer possible. - override func didFail(error: Error) { - super.didFail(error: error) - } } -class OWSContactDiscoveryBatchOperation: OWSOperation { +class LegacyContactDiscoveryBatchOperation: OWSOperation { private let recipientIdsToLookup: [String] var registeredRecipientIds: Set @@ -131,6 +106,7 @@ class OWSContactDiscoveryBatchOperation: OWSOperation { } // MARK: Mandatory overrides + // Called every retry, this is where the bulk of the operation's work should go. override func run() { Logger.debug("\(logTag) in \(#function)") @@ -156,11 +132,6 @@ class OWSContactDiscoveryBatchOperation: OWSOperation { } }, failure: { (task, error) in - if (!IsNSErrorNetworkFailure(error)) { - // FIXME not accessible in swift for some reason. -// OWSProdError(OWSAnalyticsEvents.contactsErrorContactsIntersectionFailed) - } - guard let response = task.response as? HTTPURLResponse else { let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError responseError.isRetryable = true @@ -179,18 +150,94 @@ class OWSContactDiscoveryBatchOperation: OWSOperation { // MARK: Optional Overrides - // Called one time only - override func checkForPreconditionError() -> Error? { - return super.checkForPreconditionError() - } - // Called at most one time. override func didSucceed() { - super.didSucceed() + // Compare against new CDS service + let newCDSBatchOperation = CDSBatchOperation(recipientIdsToLookup: self.recipientIdsToLookup) + let cdsFeedbackOperation = CDSFeedbackOperation(legacyRegisteredRecipientIds: self.registeredRecipientIds) + cdsFeedbackOperation.addDependency(newCDSBatchOperation) + + CDSFeedbackOperation.operationQueue.addOperations([newCDSBatchOperation, cdsFeedbackOperation], waitUntilFinished: false) + } + +} + +class CDSFeedbackOperation: OWSOperation { + + static let operationQueue = OperationQueue() + + let legacyRegisteredRecipientIds: Set + + required init(legacyRegisteredRecipientIds: Set) { + self.legacyRegisteredRecipientIds = legacyRegisteredRecipientIds + + super.init() + + Logger.debug("\(logTag) in \(#function)") + } + + // MARK: Mandatory overrides + + // Called every retry, this is where the bulk of the operation's work should go. + override func run() { + guard let cdsOperation = dependencies.first as? CDSBatchOperation else { + let error = OWSErrorMakeAssertionError("\(self.logTag) in \(#function) cdsOperation was unexpectedly nil") + self.reportError(error) + return + } + + let cdsRegisteredRecipientIds = cdsOperation.registeredRecipientIds + + if cdsRegisteredRecipientIds == legacyRegisteredRecipientIds { + Logger.debug("\(logTag) in \(#function) TODO: PUT /v1/directory/feedback/ok") + } else { + Logger.debug("\(logTag) in \(#function) TODO: PUT /v1/directory/feedback/mismatch") + } + + self.reportSuccess() } - // Called at most one time, once retry is no longer possible. override func didFail(error: Error) { - super.didFail(error: error) + // dependency failed. + // Depending on error, PUT one of: + // /v1/directory/feedback/server-error: + // /v1/directory/feedback/client-error: + // /v1/directory/feedback/attestation-error: + // /v1/directory/feedback/unexpected-error: + Logger.debug("\(logTag) in \(#function) TODO: PUT /v1/directory/feedback/*-error") + } +} + +class CDSBatchOperation: OWSOperation { + + private let recipientIdsToLookup: [String] + var registeredRecipientIds: Set + + required init(recipientIdsToLookup: [String]) { + self.recipientIdsToLookup = recipientIdsToLookup + self.registeredRecipientIds = Set() + + super.init() + + Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") + } + + // MARK: Mandatory overrides + + // Called every retry, this is where the bulk of the operation's work should go. + override func run() { + Logger.debug("\(logTag) in \(#function)") + + Logger.debug("\(logTag) in \(#function) FAKING intersection (TODO)") + self.registeredRecipientIds = Set(self.recipientIdsToLookup) + self.reportSuccess() + } +} + +extension Array { + func chunked(by chunkSize: Int) -> [[Element]] { + return stride(from: 0, to: self.count, by: chunkSize).map { + Array(self[$0.. Date: Wed, 18 Jul 2018 22:36:31 -0600 Subject: [PATCH 4/8] update pods for batching contact intersections --- Pods | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Pods b/Pods index 2bace50e8..e9badd4ba 160000 --- a/Pods +++ b/Pods @@ -1 +1 @@ -Subproject commit 2bace50e8e41e281a9ddd525bc4b8a97446301ca +Subproject commit e9badd4baacc97c7159f7f8ffd2a51f4581fb869 From 9293eb96f7601672e657be7900784a637b1edacd Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 18 Jul 2018 22:42:18 -0600 Subject: [PATCH 5/8] code re-org --- .../OWSContactDiscoveryOperation.swift | 159 +++++++++--------- 1 file changed, 82 insertions(+), 77 deletions(-) diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index 6b45e0deb..d0137738f 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -7,7 +7,6 @@ import Foundation @objc(OWSContactDiscoveryOperation) class ContactDiscoveryOperation: OWSOperation { - // TODO verify proper batch size let batchSize = 2048 let recipientIdsToLookup: [String] @@ -49,9 +48,15 @@ class ContactDiscoveryOperation: OWSOperation { class LegacyContactDiscoveryBatchOperation: OWSOperation { - private let recipientIdsToLookup: [String] var registeredRecipientIds: Set + private let recipientIdsToLookup: [String] + private var networkManager: TSNetworkManager { + return TSNetworkManager.shared() + } + + // MARK: Initializers + required init(recipientIdsToLookup: [String]) { self.recipientIdsToLookup = recipientIdsToLookup self.registeredRecipientIds = Set() @@ -61,51 +66,7 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") } - private var networkManager: TSNetworkManager { - return TSNetworkManager.shared() - } - - private func parse(response: Any?, phoneNumbersByHashes: [String: String]) throws -> Set { - - guard let responseDict = response as? [String: AnyObject] else { - let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError - responseError.isRetryable = true - - throw responseError - } - - guard let contactDicts = responseDict["contacts"] as? [[String: AnyObject]] else { - let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError - responseError.isRetryable = true - - throw responseError - } - - var registeredRecipientIds: Set = Set() - - for contactDict in contactDicts { - guard let hash = contactDict["token"] as? String, hash.count > 0 else { - owsFail("\(self.logTag) in \(#function) hash was unexpectedly nil") - continue - } - - guard let recipientId = phoneNumbersByHashes[hash], recipientId.count > 0 else { - owsFail("\(self.logTag) in \(#function) recipientId was unexpectedly nil") - continue - } - - guard recipientIdsToLookup.contains(recipientId) else { - owsFail("\(self.logTag) in \(#function) unexpected recipientId") - continue - } - - registeredRecipientIds.insert(recipientId) - } - - return registeredRecipientIds - } - - // MARK: Mandatory overrides + // MARK: OWSOperation Overrides // Called every retry, this is where the bulk of the operation's work should go. override func run() { @@ -148,8 +109,6 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { }) } - // MARK: Optional Overrides - // Called at most one time. override func didSucceed() { // Compare against new CDS service @@ -160,13 +119,85 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { CDSFeedbackOperation.operationQueue.addOperations([newCDSBatchOperation, cdsFeedbackOperation], waitUntilFinished: false) } + // MARK: Private Helpers + + private func parse(response: Any?, phoneNumbersByHashes: [String: String]) throws -> Set { + + guard let responseDict = response as? [String: AnyObject] else { + let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError + responseError.isRetryable = true + + throw responseError + } + + guard let contactDicts = responseDict["contacts"] as? [[String: AnyObject]] else { + let responseError: NSError = OWSErrorMakeUnableToProcessServerResponseError() as NSError + responseError.isRetryable = true + + throw responseError + } + + var registeredRecipientIds: Set = Set() + + for contactDict in contactDicts { + guard let hash = contactDict["token"] as? String, hash.count > 0 else { + owsFail("\(self.logTag) in \(#function) hash was unexpectedly nil") + continue + } + + guard let recipientId = phoneNumbersByHashes[hash], recipientId.count > 0 else { + owsFail("\(self.logTag) in \(#function) recipientId was unexpectedly nil") + continue + } + + guard recipientIdsToLookup.contains(recipientId) else { + owsFail("\(self.logTag) in \(#function) unexpected recipientId") + continue + } + + registeredRecipientIds.insert(recipientId) + } + + return registeredRecipientIds + } + +} + +class CDSBatchOperation: OWSOperation { + + private let recipientIdsToLookup: [String] + var registeredRecipientIds: Set + + // MARK: Initializers + + required init(recipientIdsToLookup: [String]) { + self.recipientIdsToLookup = recipientIdsToLookup + self.registeredRecipientIds = Set() + + super.init() + + Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") + } + + // MARK: OWSOperationOverrides + + // Called every retry, this is where the bulk of the operation's work should go. + override func run() { + Logger.debug("\(logTag) in \(#function)") + + Logger.debug("\(logTag) in \(#function) FAKING intersection (TODO)") + self.registeredRecipientIds = Set(self.recipientIdsToLookup) + self.reportSuccess() + } } class CDSFeedbackOperation: OWSOperation { static let operationQueue = OperationQueue() - let legacyRegisteredRecipientIds: Set + private let legacyRegisteredRecipientIds: Set + + // MARK: Initializers required init(legacyRegisteredRecipientIds: Set) { self.legacyRegisteredRecipientIds = legacyRegisteredRecipientIds @@ -176,7 +207,7 @@ class CDSFeedbackOperation: OWSOperation { Logger.debug("\(logTag) in \(#function)") } - // MARK: Mandatory overrides + // MARK: OWSOperation Overrides // Called every retry, this is where the bulk of the operation's work should go. override func run() { @@ -208,32 +239,6 @@ class CDSFeedbackOperation: OWSOperation { } } -class CDSBatchOperation: OWSOperation { - - private let recipientIdsToLookup: [String] - var registeredRecipientIds: Set - - required init(recipientIdsToLookup: [String]) { - self.recipientIdsToLookup = recipientIdsToLookup - self.registeredRecipientIds = Set() - - super.init() - - Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") - } - - // MARK: Mandatory overrides - - // Called every retry, this is where the bulk of the operation's work should go. - override func run() { - Logger.debug("\(logTag) in \(#function)") - - Logger.debug("\(logTag) in \(#function) FAKING intersection (TODO)") - self.registeredRecipientIds = Set(self.recipientIdsToLookup) - self.reportSuccess() - } -} - extension Array { func chunked(by chunkSize: Int) -> [[Element]] { return stride(from: 0, to: self.count, by: chunkSize).map { From 0db339b849e6ead9a84a1073decf3bebbaae3564 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 18 Jul 2018 22:47:21 -0600 Subject: [PATCH 6/8] fixup double failure --- .../src/Contacts/OWSContactDiscoveryOperation.swift | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index d0137738f..7cf593fb3 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -100,10 +100,12 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { return } - if (response.statusCode == 413) { + guard response.statusCode != 413 else { let rateLimitError = OWSErrorWithCodeDescription(OWSErrorCode.contactsUpdaterRateLimit, "Contacts Intersection Rate Limit") self.reportError(rateLimitError) + return } + self.reportError(error) }) From 90214ae578281e16014eb2d4fc7646bf853eaf9e Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 19 Jul 2018 13:08:59 -0600 Subject: [PATCH 7/8] make contact intersection queue serial --- SignalServiceKit/src/Contacts/ContactsUpdater.m | 1 + .../src/Contacts/OWSContactDiscoveryOperation.swift | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/SignalServiceKit/src/Contacts/ContactsUpdater.m b/SignalServiceKit/src/Contacts/ContactsUpdater.m index 8117b5845..b2a01eef7 100644 --- a/SignalServiceKit/src/Contacts/ContactsUpdater.m +++ b/SignalServiceKit/src/Contacts/ContactsUpdater.m @@ -41,6 +41,7 @@ NS_ASSUME_NONNULL_BEGIN } _contactIntersectionQueue = [NSOperationQueue new]; + _contactIntersectionQueue.maxConcurrentOperationCount = 1; OWSSingletonAssert(); diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index 7cf593fb3..d630980f8 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -195,7 +195,12 @@ class CDSBatchOperation: OWSOperation { class CDSFeedbackOperation: OWSOperation { - static let operationQueue = OperationQueue() + static let operationQueue: OperationQueue = { + let queue = OperationQueue() + queue.maxConcurrentOperationCount = 1 + + return queue + }() private let legacyRegisteredRecipientIds: Set From eb4c62593b3cd6fc8682b824ad06f720df8c5d61 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 19 Jul 2018 13:38:45 -0600 Subject: [PATCH 8/8] Cancel quickly if dependent operation fails --- .../OWSContactDiscoveryOperation.swift | 28 ++++++++++++++++++- SignalServiceKit/src/Util/OWSOperation.h | 18 +++++++++--- SignalServiceKit/src/Util/OWSOperation.m | 15 ++++++++++ 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift index d630980f8..018054f7c 100644 --- a/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift +++ b/SignalServiceKit/src/Contacts/OWSContactDiscoveryOperation.swift @@ -5,7 +5,7 @@ import Foundation @objc(OWSContactDiscoveryOperation) -class ContactDiscoveryOperation: OWSOperation { +class ContactDiscoveryOperation: OWSOperation, LegacyContactDiscoveryBatchOperationDelegate { let batchSize = 2048 let recipientIdsToLookup: [String] @@ -23,6 +23,7 @@ class ContactDiscoveryOperation: OWSOperation { Logger.debug("\(logTag) in \(#function) with recipientIdsToLookup: \(recipientIdsToLookup.count)") for batchIds in recipientIdsToLookup.chunked(by: batchSize) { let batchOperation = LegacyContactDiscoveryBatchOperation(recipientIdsToLookup: batchIds) + batchOperation.delegate = self self.addDependency(batchOperation) } } @@ -44,11 +45,24 @@ class ContactDiscoveryOperation: OWSOperation { self.reportSuccess() } + + // MARK: LegacyContactDiscoveryBatchOperationDelegate + func contactDiscoverBatchOperation(_ contactDiscoverBatchOperation: LegacyContactDiscoveryBatchOperation, didFailWithError error: Error) { + Logger.debug("\(logTag) in \(#function) canceling self and all dependencies.") + + self.dependencies.forEach { $0.cancel() } + self.cancel() + } +} + +protocol LegacyContactDiscoveryBatchOperationDelegate: class { + func contactDiscoverBatchOperation(_ contactDiscoverBatchOperation: LegacyContactDiscoveryBatchOperation, didFailWithError error: Error) } class LegacyContactDiscoveryBatchOperation: OWSOperation { var registeredRecipientIds: Set + weak var delegate: LegacyContactDiscoveryBatchOperationDelegate? private let recipientIdsToLookup: [String] private var networkManager: TSNetworkManager { @@ -72,10 +86,17 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { override func run() { Logger.debug("\(logTag) in \(#function)") + guard !isCancelled else { + Logger.info("\(logTag) in \(#function) no work to do, since we were canceled") + self.reportCancelled() + return + } + var phoneNumbersByHashes: [String: String] = [:] for recipientId in recipientIdsToLookup { let hash = Cryptography.truncatedSHA1Base64EncodedWithoutPadding(recipientId) + assert(phoneNumbersByHashes[hash] == nil) phoneNumbersByHashes[hash] = recipientId } @@ -121,6 +142,11 @@ class LegacyContactDiscoveryBatchOperation: OWSOperation { CDSFeedbackOperation.operationQueue.addOperations([newCDSBatchOperation, cdsFeedbackOperation], waitUntilFinished: false) } + // Called at most one time. + override func didFail(error: Error) { + self.delegate?.contactDiscoverBatchOperation(self, didFailWithError: error) + } + // MARK: Private Helpers private func parse(response: Any?, phoneNumbersByHashes: [String: String]) throws -> Set { diff --git a/SignalServiceKit/src/Util/OWSOperation.h b/SignalServiceKit/src/Util/OWSOperation.h index c86c90394..55e30bed0 100644 --- a/SignalServiceKit/src/Util/OWSOperation.h +++ b/SignalServiceKit/src/Util/OWSOperation.h @@ -42,17 +42,27 @@ typedef NS_ENUM(NSInteger, OWSOperationState) { // Called at most one time. - (void)didSucceed; +// Called at most one time. +- (void)didCancel; + // Called at most one time, once retry is no longer possible. - (void)didFailWithError:(NSError *)error NS_SWIFT_NAME(didFail(error:)); #pragma mark - Success/Error - Do Not Override -// Complete the operation successfully. -// Should be called at most once per operation instance. -// You must ensure that `run` cannot fail after calling `reportSuccess`. +// Report that the operation completed successfully. +// +// Each invocation of `run` must make exactly one call to one of: `reportSuccess`, `reportCancelled`, or `reportError:` - (void)reportSuccess; -// Should be called at most once per `run`. +// Call this when you abort before completion due to being cancelled. +// +// Each invocation of `run` must make exactly one call to one of: `reportSuccess`, `reportCancelled`, or `reportError:` +- (void)reportCancelled; + +// Report that the operation failed to complete due to an error. +// +// Each invocation of `run` must make exactly one call to one of: `reportSuccess`, `reportCancelled`, or `reportError:` // You must ensure that `run` cannot succeed after calling `reportError`, e.g. generally you'll write something like // this: // diff --git a/SignalServiceKit/src/Util/OWSOperation.m b/SignalServiceKit/src/Util/OWSOperation.m index d70cbb4c6..da9fa32e8 100644 --- a/SignalServiceKit/src/Util/OWSOperation.m +++ b/SignalServiceKit/src/Util/OWSOperation.m @@ -81,6 +81,13 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; // Override in subclass if necessary } +// Called at most one time. +- (void)didCancel +{ + // no-op + // Override in subclass if necessary +} + // Called at most one time, once retry is no longer possible. - (void)didFailWithError:(NSError *)error { @@ -113,6 +120,14 @@ NSString *const OWSOperationKeyIsFinished = @"isFinished"; [self markAsComplete]; } +// These methods are not intended to be subclassed +- (void)reportCancelled +{ + DDLogDebug(@"%@ cancelled.", self.logTag); + [self didCancel]; + [self markAsComplete]; +} + - (void)reportError:(NSError *)error { DDLogDebug(@"%@ reportError: %@, fatal?: %d, retryable?: %d, remainingRetries: %lu",