Merge branch 'charlesmchen/sessionManagerPools'

SignalServiceKit/src/Network/API/TSNetworkManager.m

@@ -7,6 +7,7 @@
 #import "NSError+messageSending.h"
 #import "NSURLSessionDataTask+StatusCode.h"
 #import "OWSError.h"
+#import "OWSQueues.h"
 #import "OWSSignalService.h"
 #import "SSKEnvironment.h"
 #import "TSAccountManager.h"
@@ -25,31 +26,207 @@ BOOL IsNSErrorNetworkFailure(NSError *_Nullable error)
         && error.code == TSNetworkManagerErrorFailedConnection);
 }
 
-@interface TSNetworkManager ()
+dispatch_queue_t NetworkManagerQueue()
+{
+    static dispatch_queue_t serialQueue;
+    static dispatch_once_t onceToken;
+    dispatch_once(&onceToken, ^{
+        serialQueue = dispatch_queue_create("org.whispersystems.networkManager", DISPATCH_QUEUE_SERIAL);
+    });
+    return serialQueue;
+}
 
-// This property should only be accessed on udSerialQueue.
+#pragma mark -
-@property (atomic, readonly) AFHTTPSessionManager *udSessionManager;
-@property (atomic, readonly) NSDictionary *udSessionManagerDefaultHeaders;
 
-@property (atomic, readonly) dispatch_queue_t udSerialQueue;
+@interface OWSSessionManager : NSObject
 
-typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
+@property (nonatomic, readonly) AFHTTPSessionManager *sessionManager;
+@property (nonatomic, readonly) NSDictionary *defaultHeaders;
 
 @end
 
-@implementation TSNetworkManager
+#pragma mark -
+
+@implementation OWSSessionManager
 
 #pragma mark - Dependencies
 
-+ (TSAccountManager *)tsAccountManager
+- (OWSSignalService *)signalService
 {
-    return TSAccountManager.sharedInstance;
+    return [OWSSignalService sharedInstance];
+}
+
+#pragma mark -
+
+- (instancetype)init
+{
+    AssertOnDispatchQueue(NetworkManagerQueue());
+
+    self = [super init];
+    if (!self) {
+        return self;
+    }
+
+    _sessionManager = [self.signalService buildSignalServiceSessionManager];
+    self.sessionManager.completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+    // NOTE: We could enable HTTPShouldUsePipelining here.
+    // Make a copy of the default headers for this session manager.
+    _defaultHeaders = [self.sessionManager.requestSerializer.HTTPRequestHeaders copy];
+
+    return self;
+}
+
+//  TSNetworkManager.serialQueue
+- (void)performRequest:(TSRequest *)request
+            canUseAuth:(BOOL)canUseAuth
+               success:(TSNetworkManagerSuccess)success
+               failure:(TSNetworkManagerFailure)failure
+{
+    AssertOnDispatchQueue(NetworkManagerQueue());
+    OWSAssertDebug(request);
+    OWSAssertDebug(success);
+    OWSAssertDebug(failure);
+
+    // Clear all headers so that we don't retain headers from previous requests.
+    for (NSString *headerField in self.sessionManager.requestSerializer.HTTPRequestHeaders.allKeys.copy) {
+        [self.sessionManager.requestSerializer setValue:nil forHTTPHeaderField:headerField];
+    }
+
+    // Apply the default headers for this session manager.
+    for (NSString *headerField in self.defaultHeaders) {
+        NSString *headerValue = self.defaultHeaders[headerField];
+        [self.sessionManager.requestSerializer setValue:headerValue forHTTPHeaderField:headerField];
+    }
+
+    if (canUseAuth && request.shouldHaveAuthorizationHeaders) {
+        [self.sessionManager.requestSerializer setAuthorizationHeaderFieldWithUsername:request.authUsername
+                                                                              password:request.authPassword];
+    }
+
+    // Honor the request's headers.
+    for (NSString *headerField in request.allHTTPHeaderFields) {
+        NSString *headerValue = request.allHTTPHeaderFields[headerField];
+        [self.sessionManager.requestSerializer setValue:headerValue forHTTPHeaderField:headerField];
+    }
+
+    if ([request.HTTPMethod isEqualToString:@"GET"]) {
+        [self.sessionManager GET:request.URL.absoluteString
+                      parameters:request.parameters
+                        progress:nil
+                         success:success
+                         failure:failure];
+    } else if ([request.HTTPMethod isEqualToString:@"POST"]) {
+        [self.sessionManager POST:request.URL.absoluteString
+                       parameters:request.parameters
+                         progress:nil
+                          success:success
+                          failure:failure];
+    } else if ([request.HTTPMethod isEqualToString:@"PUT"]) {
+        [self.sessionManager PUT:request.URL.absoluteString
+                      parameters:request.parameters
+                         success:success
+                         failure:failure];
+    } else if ([request.HTTPMethod isEqualToString:@"DELETE"]) {
+        [self.sessionManager DELETE:request.URL.absoluteString
+                         parameters:request.parameters
+                            success:success
+                            failure:failure];
+    } else {
+        OWSLogError(@"Trying to perform HTTP operation with unknown verb: %@", request.HTTPMethod);
+    }
+}
+
+@end
+
+#pragma mark -
+
+// You might be asking: "why use a pool at all? We're only using the session manager
+// on the serial queue, so can't we just have two session managers (1 UD, 1 non-UD)
+// that we use for all requests?"
+//
+// That assumes that the session managers are not stateful in a way where concurrent
+// requests can interfere with each other. I audited the AFNetworking codebase and my
+// reading is that sessions managers are safe to use in that way - that the state of
+// their properties (e.g. header values) is only used when building the request and
+// can be safely changed after performRequest is complete.
+//
+// But I decided that I didn't want to (silently) bake that assumption into the
+// codebase, since the stakes are high. The session managers aren't expensive. IMO
+// better to use a pool and not re-use a session manager until its request succeeds
+// or fails.
+@interface OWSSessionManagerPool : NSObject
+
+@property (nonatomic) NSMutableArray<OWSSessionManager *> *pool;
+
+@end
+
+#pragma mark -
+
+@implementation OWSSessionManagerPool
+
+- (instancetype)init
+{
+    self = [super init];
+    if (!self) {
+        return self;
+    }
+
+    self.pool = [NSMutableArray new];
+
+    return self;
+}
+
+- (OWSSessionManager *)get
+{
+    AssertOnDispatchQueue(NetworkManagerQueue());
+
+    OWSSessionManager *_Nullable sessionManager = [self.pool lastObject];
+    if (sessionManager) {
+        OWSLogVerbose(@"Cache hit.");
+        [self.pool removeLastObject];
+    } else {
+        OWSLogVerbose(@"Cache miss.");
+        sessionManager = [OWSSessionManager new];
+    }
+    OWSAssertDebug(sessionManager);
+    return sessionManager;
+}
+
+- (void)returnToPool:(OWSSessionManager *)sessionManager
+{
+    AssertOnDispatchQueue(NetworkManagerQueue());
+
+    OWSAssertDebug(sessionManager);
+    const NSUInteger kMaxPoolSize = 3;
+    if (self.pool.count >= kMaxPoolSize) {
+        // Discard
+        return;
+    }
+    [self.pool addObject:sessionManager];
 }
 
+@end
+
+#pragma mark -
+
+@interface TSNetworkManager ()
+
+// These properties should only be accessed on serialQueue.
+@property (atomic, readonly) OWSSessionManagerPool *udSessionManagerPool;
+@property (atomic, readonly) OWSSessionManagerPool *nonUdSessionManagerPool;
+
+@end
+
 #pragma mark -
 
-@synthesize udSessionManager = _udSessionManager;
+@implementation TSNetworkManager
-@synthesize udSerialQueue = _udSerialQueue;
+
+#pragma mark - Dependencies
+
++ (TSAccountManager *)tsAccountManager
+{
+    return TSAccountManager.sharedInstance;
+}
 
 #pragma mark - Singleton
 
@@ -67,7 +244,8 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
         return self;
     }
 
-    _udSerialQueue = dispatch_queue_create("org.whispersystems.networkManager.udQueue", DISPATCH_QUEUE_SERIAL);
+    _udSessionManagerPool = [OWSSessionManagerPool new];
+    _nonUdSessionManagerPool = [OWSSessionManagerPool new];
 
     OWSSingletonAssert();
 
@@ -85,155 +263,73 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
 
 - (void)makeRequest:(TSRequest *)request
     completionQueue:(dispatch_queue_t)completionQueue
-            success:(TSNetworkManagerSuccess)successBlock
+            success:(TSNetworkManagerSuccess)success
-            failure:(TSNetworkManagerFailure)failureBlock
+            failure:(TSNetworkManagerFailure)failure
 {
     OWSAssertDebug(request);
-    OWSAssertDebug(successBlock);
+    OWSAssertDebug(success);
-    OWSAssertDebug(failureBlock);
+    OWSAssertDebug(failure);
 
-    if (request.isUDRequest) {
+    dispatch_async(NetworkManagerQueue(), ^{
-        dispatch_async(self.udSerialQueue, ^{
+        [self makeRequestSync:request completionQueue:completionQueue success:success failure:failure];
-            [self makeUDRequestSync:request success:successBlock failure:failureBlock];
-        });
-    } else {
-        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-            [self makeRequestSync:request completionQueue:completionQueue success:successBlock failure:failureBlock];
     });
-    }
 }
 
 - (void)makeRequestSync:(TSRequest *)request
         completionQueue:(dispatch_queue_t)completionQueue
-                success:(TSNetworkManagerSuccess)successBlock
+                success:(TSNetworkManagerSuccess)successParam
-                failure:(TSNetworkManagerFailure)failureBlock
+                failure:(TSNetworkManagerFailure)failureParam
 {
     OWSAssertDebug(request);
-    OWSAssertDebug(successBlock);
+    OWSAssertDebug(successParam);
-    OWSAssertDebug(failureBlock);
+    OWSAssertDebug(failureParam);
 
-    OWSLogInfo(@"Making Non-UD request: %@", request);
+    BOOL isUDRequest = request.isUDRequest;
-
+    NSString *label = (isUDRequest ? @"UD request" : @"Non-UD request");
-    // TODO: Remove this logging when the call connection issues have been resolved.
+    BOOL canUseAuth = !isUDRequest;
-    TSNetworkManagerSuccess success = ^(NSURLSessionDataTask *task, _Nullable id responseObject) {
+    if (isUDRequest) {
-        OWSLogInfo(@"Non-UD request succeeded : %@", request);
+        OWSAssert(!request.shouldHaveAuthorizationHeaders);
-
-        if (request.shouldHaveAuthorizationHeaders) {
-            [TSNetworkManager.tsAccountManager setIsDeregistered:NO];
     }
+    OWSLogInfo(@"Making %@: %@", label, request);
 
-        successBlock(task, responseObject);
+    OWSSessionManagerPool *sessionManagerPool
+        = (isUDRequest ? self.udSessionManagerPool : self.nonUdSessionManagerPool);
+    OWSSessionManager *sessionManager = [sessionManagerPool get];
 
-        [OutageDetection.sharedManager reportConnectionSuccess];
+    TSNetworkManagerSuccess success = ^(NSURLSessionDataTask *task, _Nullable id responseObject) {
-    };
+        dispatch_async(NetworkManagerQueue(), ^{
-    TSNetworkManagerFailure failure = [TSNetworkManager errorPrettifyingForFailureBlock:failureBlock request:request];
+            [sessionManagerPool returnToPool:sessionManager];
-
+        });
-    AFHTTPSessionManager *sessionManager = [OWSSignalService sharedInstance].signalServiceSessionManager;
-    // [OWSSignalService signalServiceSessionManager] always returns a new instance of
-    // session manager, so its safe to reconfigure it here.
-    sessionManager.completionQueue = completionQueue;
-
-    if (request.shouldHaveAuthorizationHeaders) {
-        [sessionManager.requestSerializer setAuthorizationHeaderFieldWithUsername:request.authUsername
-                                                                         password:request.authPassword];
-    }
-
-    // Honor the request's headers.
-    for (NSString *headerField in request.allHTTPHeaderFields) {
-        NSString *headerValue = request.allHTTPHeaderFields[headerField];
-        [sessionManager.requestSerializer setValue:headerValue forHTTPHeaderField:headerField];
-    }
 
-    [self performRequest:request sessionManager:sessionManager success:success failure:failure];
+        dispatch_async(completionQueue, ^{
-}
+            OWSLogInfo(@"%@ succeeded : %@", label, request);
 
-// This method should only be invoked on udSerialQueue.
+            if (canUseAuth && request.shouldHaveAuthorizationHeaders) {
-- (AFHTTPSessionManager *)udSessionManager
+                [TSNetworkManager.tsAccountManager setIsDeregistered:NO];
-{
-    if (!_udSessionManager) {
-        AFHTTPSessionManager *udSessionManager = [OWSSignalService sharedInstance].signalServiceSessionManager;
-        udSessionManager.completionQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
-        // NOTE: We could enable HTTPShouldUsePipelining here.
-        _udSessionManager = udSessionManager;
-        // Make a copy of the default headers for this session manager.
-        _udSessionManagerDefaultHeaders = [udSessionManager.requestSerializer.HTTPRequestHeaders copy];
             }
 
-    return _udSessionManager;
+            successParam(task, responseObject);
-}
-
-- (void)makeUDRequestSync:(TSRequest *)request
-                  success:(TSNetworkManagerSuccess)successBlock
-                  failure:(TSNetworkManagerFailure)failureBlock
-{
-    OWSAssertDebug(request);
-    OWSAssert(!request.shouldHaveAuthorizationHeaders);
-    OWSAssertDebug(successBlock);
-    OWSAssertDebug(failureBlock);
-
-    OWSLogInfo(@"Making UD request: %@", request);
-
-    TSNetworkManagerSuccess success = ^(NSURLSessionDataTask *task, _Nullable id responseObject) {
-        OWSLogInfo(@"UD request succeeded : %@", request);
-
-        successBlock(task, responseObject);
 
             [OutageDetection.sharedManager reportConnectionSuccess];
+        });
     };
-    TSNetworkManagerFailure failure = [TSNetworkManager errorPrettifyingForFailureBlock:failureBlock request:request];
+    TSNetworkManagerSuccess failure = ^(NSURLSessionDataTask *task, NSError *error) {
-
+        dispatch_async(NetworkManagerQueue(), ^{
-    AFHTTPSessionManager *sessionManager = self.udSessionManager;
+            [sessionManagerPool returnToPool:sessionManager];
+        });
 
-    // Clear all headers so that we don't retain headers from previous requests.
+        [TSNetworkManager
-    for (NSString *headerField in sessionManager.requestSerializer.HTTPRequestHeaders.allKeys.copy) {
+            handleNetworkFailure:^(NSURLSessionDataTask *task, NSError *error) {
-        [sessionManager.requestSerializer setValue:nil forHTTPHeaderField:headerField];
+                dispatch_async(completionQueue, ^{
-    }
+                    failureParam(task, error);
-    // Apply the default headers for this session manager.
+                });
-    for (NSString *headerField in self.udSessionManagerDefaultHeaders) {
-        NSString *headerValue = self.udSessionManagerDefaultHeaders[headerField];
-        [sessionManager.requestSerializer setValue:headerValue forHTTPHeaderField:headerField];
-    }
-    // Honor the request's headers.
-    for (NSString *headerField in request.allHTTPHeaderFields) {
-        NSString *headerValue = request.allHTTPHeaderFields[headerField];
-        [sessionManager.requestSerializer setValue:headerValue forHTTPHeaderField:headerField];
             }
+                         request:request
+                            task:task
+                           error:error];
+    };
 
-    [self performRequest:request sessionManager:sessionManager success:success failure:failure];
+    [sessionManager performRequest:request canUseAuth:canUseAuth success:success failure:failure];
-}
-
-- (void)performRequest:(TSRequest *)request
-        sessionManager:(AFHTTPSessionManager *)sessionManager
-               success:(TSNetworkManagerSuccess)success
-               failure:(TSNetworkManagerFailure)failure
-{
-    OWSAssertDebug(request);
-    OWSAssertDebug(sessionManager);
-    OWSAssertDebug(success);
-    OWSAssertDebug(failure);
-
-    if ([request.HTTPMethod isEqualToString:@"GET"]) {
-        [sessionManager GET:request.URL.absoluteString
-                 parameters:request.parameters
-                   progress:nil
-                    success:success
-                    failure:failure];
-    } else if ([request.HTTPMethod isEqualToString:@"POST"]) {
-        [sessionManager POST:request.URL.absoluteString
-                  parameters:request.parameters
-                    progress:nil
-                     success:success
-                     failure:failure];
-    } else if ([request.HTTPMethod isEqualToString:@"PUT"]) {
-        [sessionManager PUT:request.URL.absoluteString parameters:request.parameters success:success failure:failure];
-    } else if ([request.HTTPMethod isEqualToString:@"DELETE"]) {
-        [sessionManager DELETE:request.URL.absoluteString
-                    parameters:request.parameters
-                       success:success
-                       failure:failure];
-    } else {
-        OWSLogError(@"Trying to perform HTTP operation with unknown verb: %@", request.HTTPMethod);
-    }
 }
 
 #ifdef DEBUG
@@ -276,12 +372,16 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
 }
 #endif
 
-+ (failureBlock)errorPrettifyingForFailureBlock:(failureBlock)failureBlock request:(TSRequest *)request
++ (void)handleNetworkFailure:(TSNetworkManagerFailure)failureBlock
+                     request:(TSRequest *)request
+                        task:(NSURLSessionDataTask *)task
+                       error:(NSError *)networkError
 {
     OWSAssertDebug(failureBlock);
     OWSAssertDebug(request);
+    OWSAssertDebug(task);
+    OWSAssertDebug(networkError);
 
-    return ^(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull networkError) {
     NSInteger statusCode = [task statusCode];
 
 #ifdef DEBUG
@@ -312,8 +412,7 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
             break;
         }
         case 400: {
-              OWSLogError(
+            OWSLogError(@"The request contains an invalid parameter : %@, %@", networkError.debugDescription, request);
-                  @"The request contains an invalid parameter : %@, %@", networkError.debugDescription, request);
 
             error.isRetryable = NO;
 
@@ -344,10 +443,8 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
             break;
         }
         case 411: {
-              OWSLogInfo(
+            OWSLogInfo(@"Multi-device pairing: %ld, %@, %@", (long)statusCode, networkError.debugDescription, request);
-                  @"Multi-device pairing: %ld, %@, %@", (long)statusCode, networkError.debugDescription, request);
+            NSError *customError = [self errorWithHTTPCode:statusCode
-              NSError *customError =
-                  [self errorWithHTTPCode:statusCode
                                                description:NSLocalizedString(@"MULTIDEVICE_PAIRING_MAX_DESC",
                                                                @"alert title: cannot link - reached max linked devices")
                                              failureReason:networkError.localizedFailureReason
@@ -396,7 +493,6 @@ typedef void (^failureBlock)(NSURLSessionDataTask *task, NSError *error);
             break;
         }
     }
-    };
 }
 
 + (void)deregisterAfterAuthErrorIfNecessary:(NSURLSessionDataTask *)task

SignalServiceKit/src/Network/OWSSignalService.h

@@ -1,5 +1,5 @@
 //
-//  Copyright (c) 2018 Open Whisper Systems. All rights reserved.
+//  Copyright (c) 2019 Open Whisper Systems. All rights reserved.
 //
 
 NS_ASSUME_NONNULL_BEGIN
@@ -12,9 +12,6 @@ extern NSString *const kNSNotificationName_IsCensorshipCircumventionActiveDidCha
 
 @interface OWSSignalService : NSObject
 
-/// For interacting with the Signal Service
-@property (nonatomic, readonly) AFHTTPSessionManager *signalServiceSessionManager;
-
 /// For uploading avatar assets.
 @property (nonatomic, readonly) AFHTTPSessionManager *CDNSessionManager;
 
@@ -29,6 +26,9 @@ extern NSString *const kNSNotificationName_IsCensorshipCircumventionActiveDidCha
 @property (atomic) BOOL isCensorshipCircumventionManuallyActivated;
 @property (atomic, nullable) NSString *manualCensorshipCircumventionCountryCode;
 
+/// For interacting with the Signal Service
+- (AFHTTPSessionManager *)buildSignalServiceSessionManager;
+
 @end
 
 NS_ASSUME_NONNULL_END

SignalServiceKit/src/Network/OWSSignalService.m

@@ -1,5 +1,5 @@
 //
-//  Copyright (c) 2018 Open Whisper Systems. All rights reserved.
+//  Copyright (c) 2019 Open Whisper Systems. All rights reserved.
 //
 
 #import "OWSSignalService.h"
@@ -147,7 +147,7 @@ NSString *const kNSNotificationName_IsCensorshipCircumventionActiveDidChange =
     }
 }
 
-- (AFHTTPSessionManager *)signalServiceSessionManager
+- (AFHTTPSessionManager *)buildSignalServiceSessionManager
 {
     if (self.isCensorshipCircumventionActive) {
         OWSLogInfo(@"using reflector HTTPSessionManager via: %@", self.censorshipConfiguration.domainFrontBaseURL);