From 0ab6bcd080d5268e7305351741ca8bda6cab32e7 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 11 Apr 2017 16:57:28 -0400 Subject: [PATCH] Rework outgoing message state. // FREEBIE --- src/Devices/OWSRecordTranscriptJob.m | 8 +- src/Messages/Interactions/TSOutgoingMessage.h | 46 ++++- src/Messages/Interactions/TSOutgoingMessage.m | 178 +++++++++++++++++- src/Messages/OWSFailedMessagesJob.m | 3 +- src/Messages/OWSMessageSender.m | 91 +++------ src/Messages/TSMessagesManager.m | 4 +- 6 files changed, 245 insertions(+), 85 deletions(-) diff --git a/src/Devices/OWSRecordTranscriptJob.m b/src/Devices/OWSRecordTranscriptJob.m index 134a90c47..d6940e9ae 100644 --- a/src/Devices/OWSRecordTranscriptJob.m +++ b/src/Devices/OWSRecordTranscriptJob.m @@ -1,5 +1,6 @@ -// Created by Michael Kirk on 9/23/16. -// Copyright © 2016 Open Whisper Systems. All rights reserved. +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// #import "OWSRecordTranscriptJob.h" #import "OWSAttachmentsProcessor.h" @@ -85,8 +86,7 @@ NS_ASSUME_NONNULL_BEGIN attachmentIds:[NSMutableArray new] expiresInSeconds:transcript.expirationDuration expireStartedAt:transcript.expirationStartedAt]; - textMessage.messageState = TSOutgoingMessageStateDelivered; - [textMessage save]; + [textMessage updateWithWasDelivered]; } } diff --git a/src/Messages/Interactions/TSOutgoingMessage.h b/src/Messages/Interactions/TSOutgoingMessage.h index 4c236a4a3..71e510cda 100644 --- a/src/Messages/Interactions/TSOutgoingMessage.h +++ b/src/Messages/Interactions/TSOutgoingMessage.h @@ -19,11 +19,11 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) { TSOutgoingMessageStateAttemptingOut, // The failure state. TSOutgoingMessageStateUnsent, - // The message has been sent to the service, but not received by any recipient client. - TSOutgoingMessageStateSent, - // The message has been sent to the service and received by at least one recipient client. - // A recipient may have more than one client, and group message may have more than one recipient. - TSOutgoingMessageStateDelivered + // These two enum values have been combined into TSOutgoingMessageStateSentToService. + TSOutgoingMessageStateSent_OBSOLETE, + TSOutgoingMessageStateDelivered_OBSOLETE, + // The message has been sent to the service. + TSOutgoingMessageStateSentToService, }; - (instancetype)initWithTimestamp:(uint64_t)timestamp; @@ -53,9 +53,14 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) { - (instancetype)initWithCoder:(NSCoder *)coder NS_DESIGNATED_INITIALIZER; -@property (nonatomic) TSOutgoingMessageState messageState; -@property BOOL hasSyncedTranscript; -@property NSString *customMessage; +@property (atomic, readonly) TSOutgoingMessageState messageState; + +// The message has been sent to the service and received by at least one recipient client. +// A recipient may have more than one client, and group message may have more than one recipient. +@property (atomic, readonly) BOOL wasDelivered; + +@property (atomic, readonly) BOOL hasSyncedTranscript; +@property (atomic, readonly) NSString *customMessage; @property (atomic, readonly) NSString *mostRecentFailureText; // A map of attachment id-to-filename. @property (nonatomic, readonly) NSMutableDictionary *attachmentFilenameMap; @@ -67,8 +72,6 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) { */ @property (nonatomic, readonly) BOOL isLegacyMessage; -- (void)setSendingError:(NSError *)error; - /** * Signal Identifier (e.g. e164 number) or nil if in a group thread. */ @@ -105,6 +108,29 @@ typedef NS_ENUM(NSInteger, TSOutgoingMessageState) { - (OWSSignalServiceProtosAttachmentPointer *)buildAttachmentProtoForAttachmentId:(NSString *)attachmentId filename:(nullable NSString *)filename; +// TSOutgoingMessage are updated from many threads. We don't want to save +// our local copy since it may be out of date. Instead, we use these +// "updateWith..." methods to: +// +// a) Update a property of the local copy. +// b) Load an up-to-date instance of this model from from the data store. +// c) Update and save that fresh instance. +// +// This isn't a perfect arrangement, but in practice this will prevent +// data loss. +- (void)updateWithMessageState:(TSOutgoingMessageState)messageState; +- (void)updateWithSendingError:(NSError *)error; +- (void)updateWithHasSyncedTranscript:(BOOL)hasSyncedTranscript; +- (void)updateWithCustomMessage:(NSString *)customMessage; +- (void)updateWithWasDeliveredWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; +- (void)updateWithWasDelivered; + +#pragma mark - Sent Recipients + +- (BOOL)wasSentToRecipient:(NSString *)contactId; +- (void)updateWithSentRecipient:(NSString *)contactId transaction:(YapDatabaseReadWriteTransaction *)transaction; +- (void)updateWithSentRecipient:(NSString *)contactId; + @end NS_ASSUME_NONNULL_END diff --git a/src/Messages/Interactions/TSOutgoingMessage.m b/src/Messages/Interactions/TSOutgoingMessage.m index 832cb40ce..c868ac376 100644 --- a/src/Messages/Interactions/TSOutgoingMessage.m +++ b/src/Messages/Interactions/TSOutgoingMessage.m @@ -8,19 +8,56 @@ #import "TSAttachmentStream.h" #import "TSContactThread.h" #import "TSGroupThread.h" +#import +#import NS_ASSUME_NONNULL_BEGIN +NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRecipientAll"; + +@interface TSOutgoingMessage () + +@property (atomic) TSOutgoingMessageState messageState; +@property (atomic) BOOL hasSyncedTranscript; +@property (atomic) NSString *customMessage; +@property (atomic) NSString *mostRecentFailureText; +@property (atomic) BOOL wasDelivered; +// For outgoing, non-legacy group messages sent from this client, this +// contains the list of recipients to whom the message has been sent. +// +// This collection can also be tested to avoid repeat delivery to the +// same recipient. +@property (atomic) NSArray *sentRecipients; + +@end + +#pragma mark - + @implementation TSOutgoingMessage +@synthesize sentRecipients = _sentRecipients; + - (instancetype)initWithCoder:(NSCoder *)coder { self = [super initWithCoder:coder]; + if (self) { if (!_attachmentFilenameMap) { _attachmentFilenameMap = [NSMutableDictionary new]; } + + // Migrate message state. + if (_messageState == TSOutgoingMessageStateSent_OBSOLETE) { + _messageState = TSOutgoingMessageStateSentToService; + } else if (_messageState == TSOutgoingMessageStateDelivered_OBSOLETE) { + _messageState = TSOutgoingMessageStateSentToService; + _wasDelivered = YES; + } + if (!_sentRecipients) { + _sentRecipients = [NSArray new]; + } } + return self; } @@ -85,6 +122,7 @@ NS_ASSUME_NONNULL_BEGIN } _messageState = TSOutgoingMessageStateAttemptingOut; + _sentRecipients = [NSArray new]; _hasSyncedTranscript = NO; if ([thread isKindOfClass:[TSGroupThread class]]) { @@ -117,20 +155,152 @@ NS_ASSUME_NONNULL_BEGIN - (BOOL)shouldStartExpireTimer { switch (self.messageState) { - case TSOutgoingMessageStateSent: - case TSOutgoingMessageStateDelivered: + case TSOutgoingMessageStateSentToService: return self.isExpiringMessage; case TSOutgoingMessageStateAttemptingOut: case TSOutgoingMessageStateUnsent: return NO; + case TSOutgoingMessageStateSent_OBSOLETE: + case TSOutgoingMessageStateDelivered_OBSOLETE: + OWSAssert(0); + return self.isExpiringMessage; + } +} + +#pragma mark - Update Methods + +- (void)applyChangeToSelfAndLatest:(YapDatabaseReadWriteTransaction *)transaction + changeBlock:(void (^)(TSOutgoingMessage *))changeBlock +{ + OWSAssert(transaction); + + changeBlock(self); + + TSOutgoingMessage *latestMessage = + [transaction objectForKey:self.uniqueId inCollection:[TSOutgoingMessage collection]]; + if (latestMessage) { + changeBlock(latestMessage); + [latestMessage saveWithTransaction:transaction]; + } else { + // This message has not yet been saved. + [self saveWithTransaction:transaction]; + } +} + +- (void)updateWithSendingError:(NSError *)error +{ + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [self applyChangeToSelfAndLatest:transaction + changeBlock:^(TSOutgoingMessage *message) { + [message setMessageState:TSOutgoingMessageStateUnsent]; + [message setMostRecentFailureText:error.localizedDescription]; + }]; + }]; +} + +- (void)updateWithMessageState:(TSOutgoingMessageState)messageState +{ + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [self applyChangeToSelfAndLatest:transaction + changeBlock:^(TSOutgoingMessage *message) { + [message setMessageState:messageState]; + }]; + }]; +} + +- (void)updateWithHasSyncedTranscript:(BOOL)hasSyncedTranscript +{ + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [self applyChangeToSelfAndLatest:transaction + changeBlock:^(TSOutgoingMessage *message) { + [message setHasSyncedTranscript:hasSyncedTranscript]; + }]; + }]; +} + +- (void)updateWithCustomMessage:(NSString *)customMessage +{ + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [self applyChangeToSelfAndLatest:transaction + changeBlock:^(TSOutgoingMessage *message) { + [message setCustomMessage:customMessage]; + }]; + }]; +} + +- (void)updateWithWasDeliveredWithTransaction:(YapDatabaseReadWriteTransaction *)transaction +{ + OWSAssert(transaction); + [self applyChangeToSelfAndLatest:transaction + changeBlock:^(TSOutgoingMessage *message) { + [message setWasDelivered:YES]; + }]; +} + +- (void)updateWithWasDelivered +{ + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [self updateWithWasDeliveredWithTransaction:transaction]; + }]; +} + +#pragma mark - Sent Recipients + +- (NSArray *)sentRecipients +{ + @synchronized(self) + { + return _sentRecipients; + } +} + +- (void)setSentRecipients:(NSArray *)sentRecipients +{ + @synchronized(self) + { + _sentRecipients = [sentRecipients copy]; + } +} + +- (void)addSentRecipient:(NSString *)contactId +{ + @synchronized(self) + { + OWSAssert(_sentRecipients); + OWSAssert(contactId.length > 0); + + NSMutableArray *sentRecipients = [_sentRecipients mutableCopy]; + [sentRecipients addObject:contactId]; + _sentRecipients = [sentRecipients copy]; } } -- (void)setSendingError:(NSError *)error +- (BOOL)wasSentToRecipient:(NSString *)contactId { - _mostRecentFailureText = error.localizedDescription; + OWSAssert(self.sentRecipients); + OWSAssert(contactId.length > 0); + + return [self.sentRecipients containsObject:contactId]; } +- (void)updateWithSentRecipient:(NSString *)contactId transaction:(YapDatabaseReadWriteTransaction *)transaction +{ + OWSAssert(transaction); + [self applyChangeToSelfAndLatest:transaction + changeBlock:^(TSOutgoingMessage *message) { + [message addSentRecipient:contactId]; + }]; +} + +- (void)updateWithSentRecipient:(NSString *)contactId +{ + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [self updateWithSentRecipient:contactId transaction:transaction]; + }]; +} + +#pragma mark - + - (OWSSignalServiceProtosDataMessageBuilder *)dataMessageBuilder { TSThread *thread = self.thread; diff --git a/src/Messages/OWSFailedMessagesJob.m b/src/Messages/OWSFailedMessagesJob.m index b296cb59d..546a52950 100644 --- a/src/Messages/OWSFailedMessagesJob.m +++ b/src/Messages/OWSFailedMessagesJob.m @@ -84,8 +84,7 @@ static NSString *const OWSFailedMessagesJobMessageStateIndex = @"index_outoing_m } DDLogDebug(@"%@ marking message as unsent", self.tag); - message.messageState = TSOutgoingMessageStateUnsent; - [message save]; + [message updateWithMessageState:TSOutgoingMessageStateUnsent]; count++; }]; diff --git a/src/Messages/OWSMessageSender.m b/src/Messages/OWSMessageSender.m index 24f28a66c..4d28565ba 100644 --- a/src/Messages/OWSMessageSender.m +++ b/src/Messages/OWSMessageSender.m @@ -87,8 +87,6 @@ typedef NS_ENUM(NSInteger, OWSSendMessageOperationState) { success:(void (^)())successHandler failure:(RetryableFailureHandler)failureHandler; -- (void)saveMessage:(TSOutgoingMessage *)message withError:(NSError *)error; - @end #pragma mark - @@ -136,6 +134,9 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4; OWSCAssert(NO); return; } + + [message updateWithMessageState:TSOutgoingMessageStateSentToService]; + DDLogDebug(@"%@ succeeded.", strongSelf.tag); aSuccessHandler(); [strongSelf markAsComplete]; @@ -148,7 +149,7 @@ NSUInteger const OWSSendMessageOperationMaxRetries = 4; return; } - [strongSelf.messageSender saveMessage:strongSelf.message withError:error]; + [strongSelf.message updateWithSendingError:error]; DDLogDebug(@"%@ failed with error: %@", strongSelf.tag, error); aFailureHandler(error); @@ -355,7 +356,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; { AssertIsOnMainThread(); - [self saveMessage:message withState:TSOutgoingMessageStateAttemptingOut]; + [message updateWithMessageState:TSOutgoingMessageStateAttemptingOut]; OWSSendMessageOperation *sendMessageOperation = [[OWSSendMessageOperation alloc] initWithMessage:message messageSender:self success:successHandler @@ -491,36 +492,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // 2.) fail and create a new identical error message in the thread. [errorMessage remove]; - if ([errorMessage.thread isKindOfClass:[TSContactThread class]]) { - return [self sendMessage:message success:successHandler failure:failureHandler]; - } - - // else it's a GroupThread - dispatch_async([OWSDispatch sendingQueue], ^{ - - // Avoid spamming entire group when resending failed message. - SignalRecipient *failedRecipient = [SignalRecipient fetchObjectWithUniqueID:errorMessage.recipientId]; - - // 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. - 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); - }; - - [self groupSend:@[ failedRecipient ] - message:message - thread:message.thread - success:successHandler - failure:markAndFailureHandler]; - }); + return [self sendMessage:message success:successHandler failure:failureHandler]; } - (NSArray *)getRecipients:(NSArray *)identifiers error:(NSError **)error @@ -664,6 +636,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; thread:thread attempts:OWSMessageSenderRetryAttempts success:^{ + DDLogInfo(@"Marking group message as sent to recipient: %@", recipient.uniqueId); + [message updateWithSentRecipient:recipient.uniqueId]; [futureSource trySetResult:@1]; } failure:^(NSError *error, BOOL isRetryable) { @@ -683,14 +657,20 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [self saveGroupMessage:message inThread:thread]; NSMutableArray *futures = [NSMutableArray array]; - for (SignalRecipient *rec in recipients) { + for (SignalRecipient *recipient in recipients) { // We don't need to send the message to ourselves... - if ([rec.uniqueId isEqualToString:[TSStorageManager localNumber]]) { + if ([recipient.uniqueId isEqualToString:[TSStorageManager localNumber]]) { + continue; + } + if ([message wasSentToRecipient:recipient.uniqueId]) { + // Skip recipients we have already sent this message to (on an + // earlier retry, perhaps). + DDLogInfo(@"Skipping group message recipient; already sent: %@", recipient.uniqueId); continue; } // ...otherwise we send. - [futures addObject:[self sendMessageFuture:message recipient:rec thread:thread]]; + [futures addObject:[self sendMessageFuture:message recipient:recipient thread:thread]]; } TOCFuture *completionFuture = futures.toc_thenAll; @@ -938,12 +918,10 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)handleMessageSentLocally:(TSOutgoingMessage *)message { - [self saveMessage:message withState:TSOutgoingMessageStateSent]; if (message.shouldSyncTranscript) { // TODO: I suspect we shouldn't optimistically set hasSyncedTranscript. // We could set this in a success handler for [sendSyncTranscriptForMessage:]. - - message.hasSyncedTranscript = YES; + [message updateWithHasSyncedTranscript:YES]; [self sendSyncTranscriptForMessage:message]; } @@ -952,7 +930,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)handleMessageSentRemotely:(TSOutgoingMessage *)message sentAt:(uint64_t)sentAt { - [self saveMessage:message withState:TSOutgoingMessageStateDelivered]; + [message updateWithWasDelivered]; [self becomeConsistentWithDisappearingConfigurationForMessage:message]; [self.disappearingMessagesJob setExpirationForMessage:message expirationStartedAt:sentAt]; } @@ -1177,23 +1155,11 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; return TSUnknownMessageType; } -- (void)saveMessage:(TSOutgoingMessage *)message withState:(TSOutgoingMessageState)state -{ - message.messageState = state; - [message save]; -} - -- (void)saveMessage:(TSOutgoingMessage *)message withError:(NSError *)error -{ - message.messageState = TSOutgoingMessageStateUnsent; - [message setSendingError:error]; - [message save]; -} - - (void)saveGroupMessage:(TSOutgoingMessage *)message inThread:(TSThread *)thread { if (message.groupMetaMessage == TSGroupMessageDeliver) { - [self saveMessage:message withState:message.messageState]; + // TODO: Why is this necessary? + [message save]; } else if (message.groupMetaMessage == TSGroupMessageQuit) { [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp inThread:thread @@ -1239,13 +1205,14 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // TODO: This error message is never created? TSErrorMessage *errorMessage; - if (message.groupMetaMessage == TSGroupMessageNone) { - // Only update this with exception if it is not a group message as group - // messages may except for one group - // send but not another and the UI doesn't know how to handle that - [message setMessageState:TSOutgoingMessageStateUnsent]; - [message saveWithTransaction:transaction]; - } + // TODO: Is this necessary? + // if (message.groupMetaMessage == TSGroupMessageNone) { + // // Only update this with exception if it is not a group message as group + // // messages may except for one group + // // send but not another and the UI doesn't know how to handle that + // [message setMessageState:TSOutgoingMessageStateUnsent]; + // [message saveWithTransaction:transaction]; + // } [errorMessage saveWithTransaction:transaction]; }]; diff --git a/src/Messages/TSMessagesManager.m b/src/Messages/TSMessagesManager.m index 2342e2e5f..55418e1c4 100644 --- a/src/Messages/TSMessagesManager.m +++ b/src/Messages/TSMessagesManager.m @@ -237,9 +237,7 @@ NS_ASSUME_NONNULL_BEGIN [TSInteraction interactionForTimestamp:envelope.timestamp withTransaction:transaction]; if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)interaction; - outgoingMessage.messageState = TSOutgoingMessageStateDelivered; - - [outgoingMessage saveWithTransaction:transaction]; + [outgoingMessage updateWithWasDeliveredWithTransaction:transaction]; } }]; }