diff --git a/src/Messages/OWSMessageSender.h b/src/Messages/OWSMessageSender.h index 3cc4653d8..371c54ddc 100644 --- a/src/Messages/OWSMessageSender.h +++ b/src/Messages/OWSMessageSender.h @@ -15,6 +15,13 @@ NS_ASSUME_NONNULL_BEGIN @class TSThread; @protocol ContactsManagerProtocol; +/** + * Useful for when you *sometimes* want to retry before giving up and calling the failure handler + * but *sometimes* we don't want to retry when we know it's a terminal failure, so we allow the + * caller to indicate this with isRetryable=NO. + */ +typedef void (^RetryableFailureHandler)(NSError *_Nonnull error, BOOL isRetryable); + NS_SWIFT_NAME(MessageSender) @interface OWSMessageSender : NSObject { diff --git a/src/Messages/OWSMessageSender.m b/src/Messages/OWSMessageSender.m index 7be6a6206..c03bde00b 100644 --- a/src/Messages/OWSMessageSender.m +++ b/src/Messages/OWSMessageSender.m @@ -79,7 +79,7 @@ typedef NS_ENUM(NSInteger, OWSSendMessageOperationState) { - (void)attemptToSendMessage:(TSOutgoingMessage *)message success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler; + failure:(RetryableFailureHandler)failureHandler; @end @@ -209,10 +209,17 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4; - (void)tryWithRemainingRetries:(NSUInteger)remainingRetries { - DDLogDebug(@"%@ remainingRetries: %lu", self.tag, remainingRetries); + DDLogDebug(@"%@ remainingRetries: %lu", self.tag, (unsigned long)remainingRetries); - void (^retryableFailureHandler)(NSError *_Nonnull) = ^(NSError *_Nonnull error) { + RetryableFailureHandler retryableFailureHandler = ^(NSError *_Nonnull error, BOOL isRetryable) { DDLogInfo(@"%@ Sending failed.", self.tag); + + if (!isRetryable) { + DDLogInfo(@"%@ Skipping retry due to terminal error: %@", self.tag, error); + self.failureHandler(error); + return; + } + if (remainingRetries > 0) { [self tryWithRemainingRetries:remainingRetries - 1]; } else { @@ -349,12 +356,13 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)attemptToSendMessage:(TSOutgoingMessage *)message success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(RetryableFailureHandler)failureHandler { DDLogDebug(@"%@ sending message: %@", self.tag, message.debugDescription); - void (^markAndFailureHandler)(NSError *error) = ^(NSError *error) { + RetryableFailureHandler markAndFailureHandler = ^(NSError *error, BOOL isRetryable) { + // TODO do we really want to mark this as failed if we're still retrying? [self saveMessage:message withError:error]; - failureHandler(error); + failureHandler(error, isRetryable); }; [self ensureAnyAttachmentsUploaded:message @@ -366,7 +374,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)ensureAnyAttachmentsUploaded:(TSOutgoingMessage *)message success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(RetryableFailureHandler)failureHandler { if (!message.hasAttachments) { DDLogDebug(@"%@ No attachments for message: %@", self.tag, message); @@ -379,7 +387,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if (!attachmentStream) { DDLogError(@"%@ Unable to find local saved attachment to upload.", self.tag); NSError *error = OWSErrorMakeFailedToSendOutgoingMessageError(); - return failureHandler(error); + // Not finding local attachment is a terminal failure. + return failureHandler(error, NO); } [self.uploadingService uploadAttachmentStream:attachmentStream @@ -471,8 +480,15 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // Normally marking as unsent is handled in sendMessage happy path, but beacuse we're skipping the common entry // point to message sending in order to send to a single recipient, we have to handle it ourselves. - void (^markAndFailureHandler)(NSError *error) = ^(NSError *error) { + RetryableFailureHandler markAndFailureHandler = ^(NSError *error, BOOL isRetryable) { [self saveMessage:message withError:error]; + if (isRetryable) { + // FIXME: Fixing this will require a larger refactor. In the meanwhile, we don't + // retry message sending from accepting key-changes in a group. (Except for the inner retry logic w/ the + // messages API) + DDLogWarn(@"%@ Skipping retry for group-message failure during %s.", self.tag, __PRETTY_FUNCTION__); + } + failureHandler(error); }; @@ -512,7 +528,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)deliverMessage:(TSOutgoingMessage *)message success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(RetryableFailureHandler)failureHandler { TSThread *thread = message.thread; @@ -526,11 +542,13 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if (recipients.count == 0) { if (error) { - failureHandler(error); + // If not recipients were found, there's no reason to retry. It will just fail again. + failureHandler(error, NO); return; } else { DDLogError(@"%@ Unknown error finding contacts", self.tag); - failureHandler(OWSErrorMakeFailedToSendOutgoingMessageError()); + // If not recipients were found, there's no reason to retry. It will just fail again. + failureHandler(OWSErrorMakeFailedToSendOutgoingMessageError(), NO); return; } } @@ -558,7 +576,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if ([blockedPhoneNumbers containsObject:recipientContactId]) { DDLogInfo(@"%@ skipping 1:1 send to blocked contact: %@", self.tag, recipientContactId); NSError *error = OWSErrorMakeMessageSendFailedToBlocklistError(); - failureHandler(error); + // No need to retry - the user will continue to be blocked. + failureHandler(error, NO); return; } @@ -575,7 +594,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } DDLogError(@"%@ contact lookup failed with error: %@", self.tag, error); - failureHandler(error); + // No need to repeat trying to find a failure. Apart from repeatedly failing, it would also cause us + // to print redundant error messages. + failureHandler(error, NO); return; } } @@ -583,7 +604,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if (!recipient) { NSError *error = OWSErrorMakeFailedToSendOutgoingMessageError(); DDLogWarn(@"recipient contact still not found after attempting lookup."); - failureHandler(error); + // No need to repeat trying to find a failure. Apart from repeatedly failing, it would also cause us to + // print redundant error messages. + failureHandler(error, NO); return; } @@ -595,8 +618,12 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; failure:failureHandler]; } else { DDLogError(@"%@ Unexpected unhandlable message: %@", self.tag, message); + + // Neither a group nor contact thread? This should never happen. + OWSAssert(NO); + NSError *error = OWSErrorMakeFailedToSendOutgoingMessageError(); - failureHandler(error); + failureHandler(error, NO); } }); } @@ -616,7 +643,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; success:^{ [futureSource trySetResult:@1]; } - failure:^(NSError *error) { + failure:^(NSError *error, BOOL isRetryable) { + // FIXME what to do WRT retryable here? [futureSource trySetFailure:error]; }]; @@ -627,7 +655,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; message:(TSOutgoingMessage *)message thread:(TSThread *)thread success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(RetryableFailureHandler)failureHandler { [self saveGroupMessage:message inThread:thread]; NSMutableArray *futures = [NSMutableArray array]; @@ -669,14 +697,16 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if (failedFuture.hasFailed) { id failureResult = failedFuture.forceGetFailure; if ([failureResult isKindOfClass:[NSError class]]) { - return failureHandler((NSError *)failureResult); + // Generally we assume that failures are retryable + return failureHandler((NSError *)failureResult, YES); } } } } DDLogWarn(@"%@ Unexpected generic failure: %@", self.tag, failure); - return failureHandler(OWSErrorMakeFailedToSendOutgoingMessageError()); + OWSAssert(NO); + return failureHandler(OWSErrorMakeFailedToSendOutgoingMessageError(), YES); }]; } @@ -696,7 +726,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; thread:(TSThread *)thread attempts:(int)remainingAttempts success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(RetryableFailureHandler)failureHandler { DDLogDebug(@"%@ sending message to service: %@", self.tag, message.debugDescription); @@ -717,13 +747,15 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; }]; DDLogError(@"%@ Message send failed due to repeated inability to update prekeys.", self.tag); - return failureHandler(OWSErrorMakeMessageSendDisabledDueToPreKeyUpdateFailuresError()); + return failureHandler(OWSErrorMakeMessageSendDisabledDueToPreKeyUpdateFailuresError(), YES); } if (remainingAttempts <= 0) { // We should always fail with a specific error. DDLogError(@"%@ Unexpected generic failure.", self.tag); - return failureHandler(OWSErrorMakeFailedToSendOutgoingMessageError()); + OWSAssert(NO); + + return failureHandler(OWSErrorMakeFailedToSendOutgoingMessageError(), YES); } remainingAttempts -= 1; @@ -741,14 +773,18 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; NSError *error = OWSErrorWithCodeDescription(OWSErrorCodeUntrustedIdentityKey, NSLocalizedString(@"FAILED_SENDING_BECAUSE_UNTRUSTED_IDENTITY_KEY", @"action sheet header when re-sending message which failed because of untrusted identity keys")); - return failureHandler(error); + // Key will continue to be unaccepted, so no need to retry. It'll only cause us to hit the Pre-Key request + // rate limit + return failureHandler(error, NO); } if ([exception.name isEqualToString:OWSMessageSenderRateLimitedException]) { NSError *error = OWSErrorWithCodeDescription(OWSErrorCodeSignalServiceRateLimited, NSLocalizedString(@"FAILED_SENDING_BECAUSE_RATE_LIMIT", @"action sheet header when re-sending message which failed because of too many attempts")); - return failureHandler(error); + + // We're already rate-limited. No need to exacerbate the problem. + return failureHandler(error, NO); } if (remainingAttempts == 0) { @@ -756,7 +792,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; @"%@ Terminal failure to build any device messages. Giving up with exception:%@", self.tag, exception); [self processException:exception outgoingMessage:message inThread:thread]; NSError *error = OWSErrorMakeFailedToSendOutgoingMessageError(); - return failureHandler(error); + // Since we've already repeatedly failed to build messages, it's unlikely that repeating the whole process + // will succeed. + return failureHandler(error, NO); } } @@ -783,7 +821,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; void (^retrySend)() = ^void() { if (remainingAttempts <= 0) { - return failureHandler(error); + // Since we've already repeatedly failed to send to the messaging API, + // it's unlikely that repeating the whole process will succeed. + return failureHandler(error, NO); } dispatch_async([OWSDispatch sendingQueue], ^{ @@ -801,12 +841,14 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; case 401: { DDLogWarn(@"%@ Unable to send due to invalid credentials. Did the user's client get de-authed by registering elsewhere?", self.tag); NSError *error = OWSErrorWithCodeDescription(OWSErrorCodeSignalServiceFailure, NSLocalizedString(@"ERROR_DESCRIPTION_SENDING_UNAUTHORIZED", @"Error message when attempting to send message")); - return failureHandler(error); + // No need to retry if we've been de-authed. + return failureHandler(error, NO); } case 404: { [self unregisteredRecipient:recipient message:message thread:thread]; NSError *error = OWSErrorMakeNoSuchSignalRecipientError(); - return failureHandler(error); + // No need to retry if the recipient is not registered. + return failureHandler(error, NO); } case 409: { // Mismatched devices @@ -817,7 +859,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [NSJSONSerialization JSONObjectWithData:responseData options:0 error:&error]; if (error) { DDLogError(@"%@ Failed to serialize response of mismatched devices: %@", self.tag, error); - return failureHandler(error); + return failureHandler(error, YES); } [self handleMismatchedDevices:serializedResponse recipient:recipient]; @@ -831,7 +873,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if (!responseData) { DDLogWarn(@"Stale devices but server didn't specify devices in response."); NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(error); + return failureHandler(error, YES); } [self handleStaleDevicesWithResponse:responseData recipientId:recipient.uniqueId]; @@ -933,7 +975,12 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; success:^{ DDLogInfo(@"Succesfully sent sync transcript."); } - failure:^(NSError *error) { + failure:^(NSError *error, BOOL isRetryable) { + // FIXME: We don't yet honor the isRetryable flag here, since sendSyncTranscriptForMessage + // isn't yet wrapped in our retryable SendMessageOperation. Addressing this would require + // a refactor to the MessageSender. Note that we *do* however continue to respect the + // OWSMessageSenderRetryAttempts, which is an "inner" retry loop, encompassing only the + // messaging API. DDLogInfo(@"Failed to send sync transcript:%@", error); }]; } diff --git a/src/Network/API/OWSUploadingService.h b/src/Network/API/OWSUploadingService.h index 8584121ba..0b5aafffe 100644 --- a/src/Network/API/OWSUploadingService.h +++ b/src/Network/API/OWSUploadingService.h @@ -2,6 +2,8 @@ // Copyright (c) 2017 Open Whisper Systems. All rights reserved. // +#import "OWSMessageSender.h" + NS_ASSUME_NONNULL_BEGIN @class TSOutgoingMessage; @@ -20,7 +22,7 @@ extern NSString *const kAttachmentUploadAttachmentIDKey; - (void)uploadAttachmentStream:(TSAttachmentStream *)attachmentStream message:(TSOutgoingMessage *)outgoingMessage success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler; + failure:(RetryableFailureHandler)failureHandler; @end diff --git a/src/Network/API/OWSUploadingService.m b/src/Network/API/OWSUploadingService.m index 6dd3b4831..2c740ee78 100644 --- a/src/Network/API/OWSUploadingService.m +++ b/src/Network/API/OWSUploadingService.m @@ -6,6 +6,7 @@ #import "Cryptography.h" #import "MIMETypeUtil.h" #import "OWSError.h" +#import "OWSMessageSender.h" #import "TSAttachmentStream.h" #import "TSNetworkManager.h" #import "TSOutgoingMessage.h" @@ -39,7 +40,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment - (void)uploadAttachmentStream:(TSAttachmentStream *)attachmentStream message:(TSOutgoingMessage *)outgoingMessage success:(void (^)())successHandler - failure:(void (^)(NSError *_Nonnull))failureHandler + failure:(RetryableFailureHandler)failureHandler { if (attachmentStream.serverId) { DDLogDebug(@"%@ Attachment previously uploaded.", self.tag); @@ -54,7 +55,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment if (![responseObject isKindOfClass:[NSDictionary class]]) { DDLogError(@"%@ unexpected response from server: %@", self.tag, responseObject); NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(error); + return failureHandler(error, YES); } NSDictionary *responseDict = (NSDictionary *)responseObject; @@ -65,7 +66,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment NSData *attachmentData = [attachmentStream readDataFromFileWithError:&error]; if (error) { DDLogError(@"%@ Failed to read attachment data with error:%@", self.tag, error); - return failureHandler(error); + return failureHandler(error, YES); } NSData *encryptionKey; @@ -95,7 +96,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment } failure:^(NSURLSessionDataTask *task, NSError *error) { DDLogError(@"%@ Failed to allocate attachment with error: %@", self.tag, error); - failureHandler(error); + failureHandler(error, YES); }]; } @@ -104,7 +105,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment location:(NSString *)location attachmentId:(NSString *)attachmentId success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(RetryableFailureHandler)failureHandler { NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL:[NSURL URLWithString:location]]; request.HTTPMethod = @"PUT"; @@ -126,7 +127,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment OWSAssert([NSThread isMainThread]); if (error) { [self fireProgressNotification:0 attachmentId:attachmentId]; - return failureHandler(error); + return failureHandler(error, YES); } NSInteger statusCode = ((NSHTTPURLResponse *)response).statusCode; @@ -134,7 +135,7 @@ NSString *const kAttachmentUploadAttachmentIDKey = @"kAttachmentUploadAttachment if (!isValidResponse) { DDLogError(@"%@ Unexpected server response: %d", self.tag, (int)statusCode); NSError *invalidResponseError = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(invalidResponseError); + return failureHandler(invalidResponseError, YES); } successHandler();