From df51523a8414025ce49f9791118341a0760013ca Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 17 Mar 2017 12:26:25 -0400 Subject: [PATCH 1/3] Serialize message sending and generalized retry logic. Previously messages could be sent out of order if (e.g.) 1. You're on a slow network 2. You send a big attachment 3. You immediately send text Generally in this scenario, the text will be sent before the attachment. Also, introduced a more general retry loop to retry on *any* failure during sending. Previously the only retry logic was around the messages API on the Signal Server. Now we'll also retry failures when allocating an attachment, or uploading an attachment. TODO: remove the now redundant retry logic in the message sender? TODO: there is still one place where we send messages directly through the MessageSender, rather than via the operation - when resending to a group due to a safety number change. This is separate logic because we were being sure to *only* resend to that one recipient. Cleaning this up would move a lot of code around. Once Signal-Desktop implements timestamp based de-duping we could shave that wart by having this troublesome codepath use NSOperation like everything else. // FREEBIE --- src/Messages/OWSMessageSender.m | 172 ++++++++++++++++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/src/Messages/OWSMessageSender.m b/src/Messages/OWSMessageSender.m index 70fcef4c5..197cf7361 100644 --- a/src/Messages/OWSMessageSender.m +++ b/src/Messages/OWSMessageSender.m @@ -41,6 +41,163 @@ NS_ASSUME_NONNULL_BEGIN +/** + * OWSSendMessageOperation encapsulates all the work associated with sending a message, e.g. uploading attachments, + * getting proper keys, + * and retrying upon failure. + * + * Used by `OWSMessageSender` to serialize message sending, ensuring that messages are emitted in the order they + * were sent. + */ +@interface OWSSendMessageOperation : NSOperation + +- (instancetype)init NS_UNAVAILABLE; +- (instancetype)initWithMessage:(TSOutgoingMessage *)message + messageSender:(OWSMessageSender *)messageSender + success:(void (^)())successHandler + failure:(void (^)(NSError *_Nonnull error))failureHandler NS_DESIGNATED_INITIALIZER; + +@end + +typedef NS_ENUM(NSInteger, OWSSendMessageOperationState) { + OWSSendMessageOperationStateNew, + OWSSendMessageOperationStateExecuting, + OWSSendMessageOperationStateFinished +}; + +@interface OWSMessageSender (OWSSendMessageOperation) + +- (void)attemptToSendMessage:(TSOutgoingMessage *)message + success:(void (^)())successHandler + failure:(void (^)(NSError *error))failureHandler; + +@end + +NSString *const OWSSendMessageOperationKeyIsExecuting = @"isExecuting"; +NSString *const OWSSendMessageOperationKeyIsFinished = @"isFinished"; + +NSUInteger const OWSSendMessageOperationMaxRetries = 4; + +@interface OWSSendMessageOperation () + +@property (nonatomic, readonly) TSOutgoingMessage *message; +@property (nonatomic, readonly) OWSMessageSender *messageSender; +@property (nonatomic, readonly) void (^successHandler)(); +@property (nonatomic, readonly) void (^failureHandler)(NSError *_Nonnull error); +@property (atomic) OWSSendMessageOperationState operationState; + +@end + +@implementation OWSSendMessageOperation + +- (instancetype)initWithMessage:(TSOutgoingMessage *)message + messageSender:(OWSMessageSender *)messageSender + success:(void (^)())aSuccessHandler + failure:(void (^)(NSError *_Nonnull error))aFailureHandler +{ + self = [super init]; + if (!self) { + return self; + } + + _operationState = OWSSendMessageOperationStateNew; + + _message = message; + _messageSender = messageSender; + + __weak typeof(self) weakSelf = self; + _successHandler = ^{ + typeof(self) strongSelf = weakSelf; + if (!strongSelf) { + return; + } + DDLogDebug(@"%@ succeeded.", strongSelf.tag); + aSuccessHandler(); + [strongSelf markAsComplete]; + }; + + _failureHandler = ^(NSError *_Nonnull error) { + typeof(self) strongSelf = weakSelf; + if (!strongSelf) { + return; + } + + DDLogDebug(@"%@ failed with error: %@", strongSelf.tag, error); + aFailureHandler(error); + [strongSelf markAsComplete]; + }; + + return self; +} + +#pragma mark - NSOperation overrides + +- (BOOL)isExecuting +{ + return self.operationState == OWSSendMessageOperationStateExecuting; +} + +- (BOOL)isFinished +{ + return self.operationState == OWSSendMessageOperationStateFinished; +} + +- (void)start +{ + [self willChangeValueForKey:OWSSendMessageOperationKeyIsExecuting]; + self.operationState = OWSSendMessageOperationStateExecuting; + [self didChangeValueForKey:OWSSendMessageOperationKeyIsExecuting]; + [self main]; +} + +- (void)main +{ + [self tryWithRemainingRetries:OWSSendMessageOperationMaxRetries]; +} + +#pragma mark - methods + +- (void)tryWithRemainingRetries:(NSUInteger)remainingRetries +{ + DDLogDebug(@"%@ remainingRetries: %lu", self.tag, remainingRetries); + + void (^retryableFailureHandler)(NSError *_Nonnull) = ^(NSError *_Nonnull error) { + DDLogInfo(@"%@ Sending failed.", self.tag); + if (remainingRetries > 0) { + [self tryWithRemainingRetries:remainingRetries - 1]; + } else { + DDLogWarn(@"%@ Too many failures. Giving up sending.", self.tag); + self.failureHandler(error); + } + }; + + [self.messageSender attemptToSendMessage:self.message success:self.successHandler failure:retryableFailureHandler]; +} + +- (void)markAsComplete +{ + [self willChangeValueForKey:OWSSendMessageOperationKeyIsExecuting]; + [self willChangeValueForKey:OWSSendMessageOperationKeyIsFinished]; + self.operationState = OWSSendMessageOperationStateFinished; + [self didChangeValueForKey:OWSSendMessageOperationKeyIsExecuting]; + [self didChangeValueForKey:OWSSendMessageOperationKeyIsFinished]; +} + +#pragma mark - Logging + ++ (NSString *)tag +{ + return [NSString stringWithFormat:@"[%@]", self.class]; +} + +- (NSString *)tag +{ + return self.class.tag; +} + +@end + + int const OWSMessageSenderRetryAttempts = 3; NSString *const OWSMessageSenderInvalidDeviceException = @"InvalidDeviceException"; NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; @@ -54,6 +211,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; @property (nonatomic, readonly) id contactsManager; @property (nonatomic, readonly) ContactsUpdater *contactsUpdater; @property (nonatomic, readonly) OWSDisappearingMessagesJob *disappearingMessagesJob; +@property (nonatomic, readonly) NSOperationQueue *sendingQueue; @end @@ -73,6 +231,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; _storageManager = storageManager; _contactsManager = contactsManager; _contactsUpdater = contactsUpdater; + _sendingQueue = [NSOperationQueue new]; + _sendingQueue.qualityOfService = NSOperationQualityOfServiceUserInitiated; + _sendingQueue.maxConcurrentOperationCount = 1; _uploadingService = [[OWSUploadingService alloc] initWithNetworkManager:networkManager]; _dbConnection = storageManager.newDatabaseConnection; @@ -84,6 +245,17 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)sendMessage:(TSOutgoingMessage *)message success:(void (^)())successHandler failure:(void (^)(NSError *error))failureHandler +{ + OWSSendMessageOperation *sendMessageOperation = [[OWSSendMessageOperation alloc] initWithMessage:message + messageSender:self + success:successHandler + failure:failureHandler]; + [self.sendingQueue addOperation:sendMessageOperation]; +} + +- (void)attemptToSendMessage:(TSOutgoingMessage *)message + success:(void (^)())successHandler + failure:(void (^)(NSError *error))failureHandler { DDLogDebug(@"%@ sending message: %@", self.tag, message.debugDescription); void (^markAndFailureHandler)(NSError *error) = ^(NSError *error) { From db15ff5e87f1f8bea281f89ea5f7c22f2d2fb369 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 17 Mar 2017 12:48:33 -0400 Subject: [PATCH 2/3] Save message before sending starts. Otherwise the message doesn't get saved until it's in the queue. Interestingly, this could also address some of the perceived lag mentioned in: https://github.com/WhisperSystems/Signal-iOS/pull/1850 // FREEBIE --- src/Messages/OWSMessageSender.m | 6 +----- src/Network/API/OWSUploadingService.m | 3 --- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Messages/OWSMessageSender.m b/src/Messages/OWSMessageSender.m index 197cf7361..f47e71976 100644 --- a/src/Messages/OWSMessageSender.m +++ b/src/Messages/OWSMessageSender.m @@ -246,6 +246,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; success:(void (^)())successHandler failure:(void (^)(NSError *error))failureHandler { + [self saveMessage:message withState:TSOutgoingMessageStateAttemptingOut]; OWSSendMessageOperation *sendMessageOperation = [[OWSSendMessageOperation alloc] initWithMessage:message messageSender:self success:successHandler @@ -339,8 +340,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [attachmentStream save]; [message.attachmentIds addObject:attachmentStream.uniqueId]; - - message.messageState = TSOutgoingMessageStateAttemptingOut; [message save]; [self sendMessage:message success:successHandler failure:failureHandler]; @@ -438,9 +437,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; || [message isKindOfClass:[OWSOutgoingSyncMessage class]]) { TSContactThread *contactThread = (TSContactThread *)thread; - - [self saveMessage:message withState:TSOutgoingMessageStateAttemptingOut]; - if ([contactThread.contactIdentifier isEqualToString:self.storageManager.localNumber] && ![message isKindOfClass:[OWSOutgoingSyncMessage class]]) { diff --git a/src/Network/API/OWSUploadingService.m b/src/Network/API/OWSUploadingService.m index a509f8804..6dd3b4831 100644 --- a/src/Network/API/OWSUploadingService.m +++ b/src/Network/API/OWSUploadingService.m @@ -41,9 +41,6 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment success:(void (^)())successHandler failure:(void (^)(NSError *_Nonnull))failureHandler { - outgoingMessage.messageState = TSOutgoingMessageStateAttemptingOut; - [outgoingMessage save]; - if (attachmentStream.serverId) { DDLogDebug(@"%@ Attachment previously uploaded.", self.tag); successHandler(outgoingMessage); From e68ee28e51fe9f1959345e84045de3288c006e82 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 20 Mar 2017 14:57:05 -0400 Subject: [PATCH 3/3] Add clarifying asserts per code review // FREEBIE --- src/Messages/OWSMessageSender.m | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Messages/OWSMessageSender.m b/src/Messages/OWSMessageSender.m index f47e71976..b45f3ce5c 100644 --- a/src/Messages/OWSMessageSender.m +++ b/src/Messages/OWSMessageSender.m @@ -43,8 +43,7 @@ NS_ASSUME_NONNULL_BEGIN /** * OWSSendMessageOperation encapsulates all the work associated with sending a message, e.g. uploading attachments, - * getting proper keys, - * and retrying upon failure. + * getting proper keys, and retrying upon failure. * * Used by `OWSMessageSender` to serialize message sending, ensuring that messages are emitted in the order they * were sent. @@ -109,6 +108,7 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4; _successHandler = ^{ typeof(self) strongSelf = weakSelf; if (!strongSelf) { + OWSAssert(NO); return; } DDLogDebug(@"%@ succeeded.", strongSelf.tag); @@ -119,6 +119,7 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4; _failureHandler = ^(NSError *_Nonnull error) { typeof(self) strongSelf = weakSelf; if (!strongSelf) { + OWSAssert(NO); return; }