From b07f466e0818053a63253d1f7818879ccf363bb5 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 1 Mar 2018 23:31:41 -0500 Subject: [PATCH 1/3] Fix redundant sync sends. --- SignalServiceKit/src/Devices/OWSDevice.h | 1 + SignalServiceKit/src/Devices/OWSDevice.m | 45 ++++++++++--------- .../src/Messages/OWSMessageSender.m | 27 +++++++++-- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/SignalServiceKit/src/Devices/OWSDevice.h b/SignalServiceKit/src/Devices/OWSDevice.h index e3b3f2259..b42e14b1b 100644 --- a/SignalServiceKit/src/Devices/OWSDevice.h +++ b/SignalServiceKit/src/Devices/OWSDevice.h @@ -17,6 +17,7 @@ extern uint32_t const OWSDevicePrimaryDeviceId; - (BOOL)mayHaveLinkedDevices:(YapDatabaseConnection *)dbConnection; - (void)setMayHaveLinkedDevices; +- (void)clearMayHaveLinkedDevicesIfNotSet; - (BOOL)hasReceivedSyncMessageInLastSeconds:(NSTimeInterval)intervalSeconds; - (void)setHasReceivedSyncMessage; diff --git a/SignalServiceKit/src/Devices/OWSDevice.m b/SignalServiceKit/src/Devices/OWSDevice.m index f47d0c133..17d7ad708 100644 --- a/SignalServiceKit/src/Devices/OWSDevice.m +++ b/SignalServiceKit/src/Devices/OWSDevice.m @@ -19,9 +19,6 @@ NSString *const kTSStorageManager_MayHaveLinkedDevices = @"kTSStorageManager_May @interface OWSDeviceManager () -// This property should only be accessed while synchronized on self. -@property (atomic, nullable) NSNumber *mayHaveLinkedDevicesCached; - @property (atomic) NSDate *lastReceivedSyncMessage; @end @@ -49,30 +46,34 @@ NSString *const kTSStorageManager_MayHaveLinkedDevices = @"kTSStorageManager_May { OWSAssert(dbConnection); - @synchronized(self) - { - if (!self.mayHaveLinkedDevicesCached) { - self.mayHaveLinkedDevicesCached = @([dbConnection boolForKey:kTSStorageManager_MayHaveLinkedDevices - inCollection:kTSStorageManager_OWSDeviceCollection - defaultValue:YES]); - } + return [dbConnection boolForKey:kTSStorageManager_MayHaveLinkedDevices + inCollection:kTSStorageManager_OWSDeviceCollection + defaultValue:YES]; +} - return [self.mayHaveLinkedDevicesCached boolValue]; - } +// In order to avoid skipping necessary sync messages, the default value +// for mayHaveLinkedDevices is YES. Once we've successfully sent a +// sync message with no device messages (e.g. the service has confirmed +// that we have no linked devices), we can set mayHaveLinkedDevices to NO +// to avoid unnecessary message sends for sync messages until we learn +// of a linked device (e.g. through the device linking UI or by receiving +// a sync message, etc.). +- (void)clearMayHaveLinkedDevicesIfNotSet +{ + // Note that we write async to avoid opening transactions within transactions. + [TSStorageManager.sharedManager.newDatabaseConnection + asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { + if (![transaction objectForKey:kTSStorageManager_MayHaveLinkedDevices + inCollection:kTSStorageManager_OWSDeviceCollection]) { + [transaction setObject:@(NO) + forKey:kTSStorageManager_MayHaveLinkedDevices + inCollection:kTSStorageManager_OWSDeviceCollection]; + } + }]; } - (void)setMayHaveLinkedDevices { - @synchronized(self) - { - if (self.mayHaveLinkedDevicesCached != nil && self.mayHaveLinkedDevicesCached.boolValue) { - // Skip redundant writes. - return; - } - - self.mayHaveLinkedDevicesCached = @(YES); - } - // Note that we write async to avoid opening transactions within transactions. [TSStorageManager.sharedManager.newDatabaseConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index bbdb82ccf..94c589eb5 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -934,9 +934,10 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } NSString *localNumber = [TSAccountManager localNumber]; - if ([localNumber isEqualToString:recipient.uniqueId]) { + BOOL isLocalNumber = [localNumber isEqualToString:recipient.uniqueId]; + if (isLocalNumber) { OWSAssert([message isKindOfClass:[OWSOutgoingSyncMessage class]]); - // Messages send to the "local number" should be sync messages. + // Messages sent to the "local number" should be sync messages. // // We can skip sending sync messages if we know that we have no linked // devices. However, we need to be sure to handle the case where the @@ -957,6 +958,11 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // 2. Check SignalRecipient's state. BOOL hasDeviceMessages = deviceMessages.count > 0; + DDLogInfo(@"%@ mayHaveLinkedDevices: %d, hasDeviceMessages: %d", + self.logTag, + mayHaveLinkedDevices, + hasDeviceMessages); + if (!mayHaveLinkedDevices && !hasDeviceMessages) { DDLogInfo(@"%@ Ignoring sync message without secondary devices: %@", self.logTag, [message class]); OWSAssert([message isKindOfClass:[OWSOutgoingSyncMessage class]]); @@ -982,13 +988,28 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; OWSAssert(deviceMessages.count > 0); } + if (deviceMessages.count == 0) { + DDLogWarn(@"%@ Sending a message with no device messages.", self.logTag); + } + TSSubmitMessageRequest *request = [[TSSubmitMessageRequest alloc] initWithRecipient:recipient.uniqueId messages:deviceMessages relay:recipient.relay timeStamp:message.timestamp]; - [self.networkManager makeRequest:request success:^(NSURLSessionDataTask *task, id responseObject) { + if (isLocalNumber && deviceMessages.count == 0) { + DDLogInfo(@"%@ Sent a message with no device messages; clearing 'mayHaveLinkedDevices'.", self.logTag); + // In order to avoid skipping necessary sync messages, the default value + // for mayHaveLinkedDevices is YES. Once we've successfully sent a + // sync message with no device messages (e.g. the service has confirmed + // that we have no linked devices), we can set mayHaveLinkedDevices to NO + // to avoid unnecessary message sends for sync messages until we learn + // of a linked device (e.g. through the device linking UI or by receiving + // a sync message, etc.). + [OWSDeviceManager.sharedManager clearMayHaveLinkedDevicesIfNotSet]; + } + dispatch_async([OWSDispatch sendingQueue], ^{ [recipient save]; [self handleMessageSentLocally:message]; From dcf7f550ae1d8ef4434542e71c2395b67e466bb9 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 1 Mar 2018 23:37:03 -0500 Subject: [PATCH 2/3] Fix redundant sync sends. --- SignalServiceKit/src/Messages/OWSMessageSender.m | 5 ----- 1 file changed, 5 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 94c589eb5..aa5a36661 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -958,11 +958,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // 2. Check SignalRecipient's state. BOOL hasDeviceMessages = deviceMessages.count > 0; - DDLogInfo(@"%@ mayHaveLinkedDevices: %d, hasDeviceMessages: %d", - self.logTag, - mayHaveLinkedDevices, - hasDeviceMessages); - if (!mayHaveLinkedDevices && !hasDeviceMessages) { DDLogInfo(@"%@ Ignoring sync message without secondary devices: %@", self.logTag, [message class]); OWSAssert([message isKindOfClass:[OWSOutgoingSyncMessage class]]); From b9458fffeff5adf3163ae3631fb55adbed739f32 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 10:53:22 -0500 Subject: [PATCH 3/3] Respond to CR. --- SignalServiceKit/src/Messages/OWSMessageSender.m | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index aa5a36661..498f59549 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -984,6 +984,16 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } if (deviceMessages.count == 0) { + // This might happen: + // + // * The first (after upgrading?) time we send a sync message to our linked devices. + // * After unlinking all linked devices. + // * After trying and failing to link a device. + // + // When we're not sure if we have linked devices, we need to try + // to send self-sync messages even if they have no device messages + // so that we can learn from the service whether or not there are + // linked devices that we don't know about. DDLogWarn(@"%@ Sending a message with no device messages.", self.logTag); }