From 8576de0618bcee7fb5dcb0b381ec1445e3fa88e7 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Tue, 3 Jul 2018 15:31:25 -0600 Subject: [PATCH 1/2] Fix: No contacts/groups after initial device link The server caches your device list on the websocket, so sending on the websocket to a just-linked device will always fail. We could close/open the websocket, but that might be disruptive in it's own way. Instead we'll closely mirror the Android approach, where WebSocket sends are attempted only one time, and failure is handled by falling back to the original REST approach. So note: we don't do any special handling of failures on the websocket (409/410). We simply retry it with REST which will handle the 409/410/etc. Consequently, we don't want to decrement our retry count for websocket sends. --- .../src/Messages/OWSMessageSender.m | 57 +++++++++++-------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index f499fe7bb..a41a90c2d 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -625,11 +625,12 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } [self sendMessageToService:message - recipient:recipient - thread:thread - attempts:OWSMessageSenderRetryAttempts - success:successHandler - failure:failureHandler]; + recipient:recipient + thread:thread + attempts:OWSMessageSenderRetryAttempts + useWebsocketIfAvailable:YES + success:successHandler + failure:failureHandler]; } else { // Neither a group nor contact thread? This should never happen. OWSFail(@"%@ Unknown message type: %@", self.logTag, NSStringFromClass([message class])); @@ -652,6 +653,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; recipient:recipient thread:thread attempts:OWSMessageSenderRetryAttempts + useWebsocketIfAvailable:YES success:^{ [futureSource trySetResult:@1]; } @@ -772,7 +774,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)sendMessageToService:(TSOutgoingMessage *)message recipient:(SignalRecipient *)recipient thread:(nullable TSThread *)thread - attempts:(int)remainingAttempts + attempts:(int)remainingAttemptsParam + useWebsocketIfAvailable:(BOOL)useWebsocketIfAvailable success:(void (^)(void))successHandler failure:(RetryableFailureHandler)failureHandler { @@ -806,7 +809,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; return failureHandler(error); } - if (remainingAttempts <= 0) { + if (remainingAttemptsParam <= 0) { // We should always fail with a specific error. OWSProdFail([OWSAnalyticsEvents messageSenderErrorGenericSendFailure]); @@ -814,7 +817,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [error setIsRetryable:YES]; return failureHandler(error); } - remainingAttempts -= 1; + int remainingAttempts = remainingAttemptsParam - 1; NSArray *deviceMessages; @try { @@ -978,7 +981,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; messages:deviceMessages relay:recipient.relay timeStamp:message.timestamp]; - if (TSSocketManager.canMakeRequests) { + if (useWebsocketIfAvailable && TSSocketManager.canMakeRequests) { [TSSocketManager.sharedManager makeRequest:request success:^(id _Nullable responseObject) { [self messageSendDidSucceed:message @@ -988,19 +991,21 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; success:successHandler]; } failure:^(NSInteger statusCode, NSData *_Nullable responseData, NSError *error) { - [self messageSendDidFail:message - recipient:recipient - thread:thread - isLocalNumber:isLocalNumber - deviceMessages:deviceMessages - remainingAttempts:remainingAttempts - statusCode:statusCode - error:error - responseData:responseData - success:successHandler - failure:failureHandler]; + // Websockets can fail in different ways, so we don't decrement remainingAttempts for websocket failure. + // Instead we fall back to REST, which will decrement retries. + // e.g. after linking a new device, sync messages will fail until the websocket re-opens. + [self sendMessageToService:message + recipient:recipient + thread:thread + attempts:remainingAttemptsParam + useWebsocketIfAvailable:NO + success:successHandler + failure:failureHandler]; }]; } else { + if (!useWebsocketIfAvailable && TSSocketManager.canMakeRequests) { + DDLogDebug(@"%@ in %s falling back to REST since first attempt failed.", self.logTag, __PRETTY_FUNCTION__); + } [self.networkManager makeRequest:request success:^(NSURLSessionDataTask *task, id responseObject) { [self messageSendDidSucceed:message @@ -1098,11 +1103,12 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; dispatch_async([OWSDispatch sendingQueue], ^{ DDLogDebug(@"%@ Retrying: %@", self.logTag, message.debugDescription); [self sendMessageToService:message - recipient:recipient - thread:thread - attempts:remainingAttempts - success:successHandler - failure:failureHandler]; + recipient:recipient + thread:thread + attempts:remainingAttempts + useWebsocketIfAvailable:NO + success:successHandler + failure:failureHandler]; }); }; @@ -1250,6 +1256,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; recipient:[SignalRecipient selfRecipient] thread:message.thread attempts:OWSMessageSenderRetryAttempts + useWebsocketIfAvailable:YES success:^{ DDLogInfo(@"Successfully sent sync transcript."); } From 1e8c7d63b85a8c6f3c0be85bc13d3fa3308bb79e Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 2 Jul 2018 10:22:29 -0600 Subject: [PATCH 2/2] clarify sync logging --- SignalServiceKit/src/Messages/OWSMessageSender.m | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index a41a90c2d..5a4ab31a5 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -949,15 +949,13 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; }); return; - } else if (mayHaveLinkedDevices) { + } else if (mayHaveLinkedDevices && !hasDeviceMessages) { // We may have just linked a new secondary device which is not yet reflected in // the SignalRecipient that corresponds to ourself. Proceed. Client should learn // of new secondary devices via 409 "Mismatched devices" response. - DDLogWarn(@"%@ sync message has no device messages but account has secondary devices.", self.logTag); - } else if (hasDeviceMessages) { + DDLogWarn(@"%@ account has secondary devices, but sync message has no device messages", self.logTag); + } else if (!mayHaveLinkedDevices && hasDeviceMessages) { OWSFail(@"%@ sync message has device messages for unknown secondary devices.", self.logTag); - } else { - // Account has secondary devices; proceed as usual. } } else { OWSAssert(deviceMessages.count > 0);