From a19882baaa70574e1472085bcae57f2166acdb2c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 13 Feb 2018 16:01:30 -0500 Subject: [PATCH 1/4] Avoid deadlocks in message sender. --- .../src/Network/API/TSNetworkManager.h | 14 ++++++++++--- .../src/Network/API/TSNetworkManager.m | 21 +++++++++++++++++-- .../src/Network/OWSSignalService.m | 1 - 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.h b/SignalServiceKit/src/Network/API/TSNetworkManager.h index 08451c573..67a016a04 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.h +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // /** @@ -27,6 +27,9 @@ extern NSString *const TSNetworkManagerDomain; BOOL IsNSErrorNetworkFailure(NSError *_Nullable error); +typedef void (^TSNetworkManagerSuccess)(NSURLSessionDataTask *task, id responseObject); +typedef void (^TSNetworkManagerFailure)(NSURLSessionDataTask *task, NSError *error); + @interface TSNetworkManager : NSObject - (instancetype)init NS_UNAVAILABLE; @@ -34,8 +37,13 @@ BOOL IsNSErrorNetworkFailure(NSError *_Nullable error); + (instancetype)sharedManager; - (void)makeRequest:(TSRequest *)request - success:(void (^)(NSURLSessionDataTask *task, id responseObject))success - failure:(void (^)(NSURLSessionDataTask *task, NSError *error))failure NS_SWIFT_NAME(makeRequest(_:success:failure:)); + success:(TSNetworkManagerSuccess)success + failure:(TSNetworkManagerFailure)failure NS_SWIFT_NAME(makeRequest(_:success:failure:)); + +- (void)makeRequest:(TSRequest *)request + shouldCompleteOnMainQueue:(BOOL)shouldCompleteOnMainQueue + success:(TSNetworkManagerSuccess)success + failure:(TSNetworkManagerFailure)failure NS_SWIFT_NAME(makeRequest(_:shouldCompleteOnMainQueue:success:failure:)); @end diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.m b/SignalServiceKit/src/Network/API/TSNetworkManager.m index c79c6ccf9..c86051f0c 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.m +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.m @@ -55,9 +55,21 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); #pragma mark Manager Methods - (void)makeRequest:(TSRequest *)request - success:(void (^)(NSURLSessionDataTask *task, id responseObject))success - failure:(void (^)(NSURLSessionDataTask *task, NSError *error))failureBlock + success:(TSNetworkManagerSuccess)success + failure:(TSNetworkManagerFailure)failure { + return [self makeRequest:request shouldCompleteOnMainQueue:YES success:success failure:failure]; +} + +- (void)makeRequest:(TSRequest *)request + shouldCompleteOnMainQueue:(BOOL)shouldCompleteOnMainQueue + success:(TSNetworkManagerSuccess)success + failure:(TSNetworkManagerFailure)failureBlock +{ + OWSAssert(request); + OWSAssert(success); + OWSAssert(failureBlock); + DDLogInfo(@"%@ Making request: %@", self.logTag, request); if (!CurrentAppContext().isMainApp) { if (![request isKindOfClass:[TSRecipientPrekeyRequest class]] @@ -73,6 +85,11 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); [TSNetworkManager errorPrettifyingForFailureBlock:failureBlock]; AFHTTPSessionManager *sessionManager = [OWSSignalService sharedInstance].signalServiceSessionManager; + if (shouldCompleteOnMainQueue) { + sessionManager.completionQueue = dispatch_get_main_queue(); + } else { + sessionManager.completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); + } if ([request isKindOfClass:[TSVerifyCodeRequest class]]) { // We plant the Authorization parameter ourselves, no need to double add. diff --git a/SignalServiceKit/src/Network/OWSSignalService.m b/SignalServiceKit/src/Network/OWSSignalService.m index 243d14dfa..4ff868e5f 100644 --- a/SignalServiceKit/src/Network/OWSSignalService.m +++ b/SignalServiceKit/src/Network/OWSSignalService.m @@ -205,7 +205,6 @@ NSString *const kNSNotificationName_IsCensorshipCircumventionActiveDidChange = sessionManager.requestSerializer = [AFJSONRequestSerializer serializer]; [sessionManager.requestSerializer setValue:self.censorshipConfiguration.signalServiceReflectorHost forHTTPHeaderField:@"Host"]; - sessionManager.responseSerializer = [AFJSONResponseSerializer serializer]; return sessionManager; From 01496b2db86a993c159e1b5230064c4a158ec49d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 13 Feb 2018 16:11:42 -0500 Subject: [PATCH 2/4] Avoid deadlocks in message sender. --- SignalServiceKit/src/Messages/OWSMessageSender.m | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 6d8320b1d..36424f815 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1303,8 +1303,14 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; __block dispatch_semaphore_t sema = dispatch_semaphore_create(0); __block PreKeyBundle *_Nullable bundle; __block NSException *_Nullable exception; - [self.networkManager makeRequest:[[TSRecipientPrekeyRequest alloc] initWithRecipient:identifier - deviceId:[deviceNumber stringValue]] + // It's not ideal that we're using a semaphore inside a read/write transaction. + // To avoid deadlock, we need to ensure that our success/failure completions + // are called _off_ the main thread. Otherwise we'll deadlock if the main + // thread is blocked on opening a transaction. + TSRequest *request = + [[TSRecipientPrekeyRequest alloc] initWithRecipient:identifier deviceId:[deviceNumber stringValue]]; + [self.networkManager makeRequest:request + shouldCompleteOnMainQueue:NO success:^(NSURLSessionDataTask *task, id responseObject) { bundle = [PreKeyBundle preKeyBundleFromDictionary:responseObject forDeviceNumber:deviceNumber]; dispatch_semaphore_signal(sema); From 888bf9256f7965ab31c3c4c12a22a0d7c1f4cc0c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 13 Feb 2018 16:12:45 -0500 Subject: [PATCH 3/4] Avoid deadlocks in message sender. --- SignalServiceKit/src/Network/API/TSNetworkManager.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.m b/SignalServiceKit/src/Network/API/TSNetworkManager.m index c86051f0c..1493eafe0 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.m +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.m @@ -85,6 +85,8 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); [TSNetworkManager errorPrettifyingForFailureBlock:failureBlock]; AFHTTPSessionManager *sessionManager = [OWSSignalService sharedInstance].signalServiceSessionManager; + // [OWSSignalService signalServiceSessionManager] always returns a new instance of + // session manager, so its safe to reconfigure it here. if (shouldCompleteOnMainQueue) { sessionManager.completionQueue = dispatch_get_main_queue(); } else { From 81522e4a23a1af2152d416d1f0227bbc8da8f3b8 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 14 Feb 2018 10:36:12 -0500 Subject: [PATCH 4/4] Respond to CR. --- SignalServiceKit/src/Messages/OWSMessageSender.m | 2 +- .../src/Network/API/TSNetworkManager.h | 9 ++++++--- .../src/Network/API/TSNetworkManager.m | 14 +++++--------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 36424f815..cb1da141c 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1310,7 +1310,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; TSRequest *request = [[TSRecipientPrekeyRequest alloc] initWithRecipient:identifier deviceId:[deviceNumber stringValue]]; [self.networkManager makeRequest:request - shouldCompleteOnMainQueue:NO + completionQueue:dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0) success:^(NSURLSessionDataTask *task, id responseObject) { bundle = [PreKeyBundle preKeyBundleFromDictionary:responseObject forDeviceNumber:deviceNumber]; dispatch_semaphore_signal(sema); diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.h b/SignalServiceKit/src/Network/API/TSNetworkManager.h index 67a016a04..94f58d498 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.h +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.h @@ -41,9 +41,12 @@ typedef void (^TSNetworkManagerFailure)(NSURLSessionDataTask *task, NSError *err failure:(TSNetworkManagerFailure)failure NS_SWIFT_NAME(makeRequest(_:success:failure:)); - (void)makeRequest:(TSRequest *)request - shouldCompleteOnMainQueue:(BOOL)shouldCompleteOnMainQueue - success:(TSNetworkManagerSuccess)success - failure:(TSNetworkManagerFailure)failure NS_SWIFT_NAME(makeRequest(_:shouldCompleteOnMainQueue:success:failure:)); + completionQueue:(dispatch_queue_t)completionQueue + success:(TSNetworkManagerSuccess)success + failure:(TSNetworkManagerFailure)failure NS_SWIFT_NAME(makeRequest(_:shouldCompleteOnMainQueue +:success +:failure +:)); @end diff --git a/SignalServiceKit/src/Network/API/TSNetworkManager.m b/SignalServiceKit/src/Network/API/TSNetworkManager.m index 1493eafe0..c43f84b09 100644 --- a/SignalServiceKit/src/Network/API/TSNetworkManager.m +++ b/SignalServiceKit/src/Network/API/TSNetworkManager.m @@ -58,13 +58,13 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); success:(TSNetworkManagerSuccess)success failure:(TSNetworkManagerFailure)failure { - return [self makeRequest:request shouldCompleteOnMainQueue:YES success:success failure:failure]; + return [self makeRequest:request completionQueue:dispatch_get_main_queue() success:success failure:failure]; } - (void)makeRequest:(TSRequest *)request - shouldCompleteOnMainQueue:(BOOL)shouldCompleteOnMainQueue - success:(TSNetworkManagerSuccess)success - failure:(TSNetworkManagerFailure)failureBlock + completionQueue:(dispatch_queue_t)completionQueue + success:(TSNetworkManagerSuccess)success + failure:(TSNetworkManagerFailure)failureBlock { OWSAssert(request); OWSAssert(success); @@ -87,11 +87,7 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error); AFHTTPSessionManager *sessionManager = [OWSSignalService sharedInstance].signalServiceSessionManager; // [OWSSignalService signalServiceSessionManager] always returns a new instance of // session manager, so its safe to reconfigure it here. - if (shouldCompleteOnMainQueue) { - sessionManager.completionQueue = dispatch_get_main_queue(); - } else { - sessionManager.completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0); - } + sessionManager.completionQueue = completionQueue; if ([request isKindOfClass:[TSVerifyCodeRequest class]]) { // We plant the Authorization parameter ourselves, no need to double add.