Request reattempt fix (#418)

* Fix Request Re-Attempt Issue

• Our SDK recently introduced HTTP reattempts for failed (500+) type requests.
• However, our SDK also had a timeout where it would give up on an HTTP request if it took too long
• This became apparent when calls to sendTags() when users had no network connection would, after 60 seconds, call the successBlock
• The success block was being called because the final re-attempt had not yet occurred, so no errors had been populated in the errors dictionary
• Fixed by (A) fixing the timeout to give enough time for all re-attempts to finish and (B) ensuring that there will be an error added to the errors dictionary if a request times out.

* Add Testing

• Adds testing to ensure that the reattempt logic works correctly

Author: Nightsd01

iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj

@@ -1147,6 +1147,11 @@
 				CLANG_WARN_SUSPICIOUS_MOVES = YES;
 				"CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer";
 				DEVELOPMENT_TEAM = 99SW8E36CT;
+				GCC_PREPROCESSOR_DEFINITIONS = (
+					"DEBUG=1",
+					"$(inherited)",
+					"OS_TEST=1",
+				);
 				INFOPLIST_FILE = UnitTests/Info.plist;
 				IPHONEOS_DEPLOYMENT_TARGET = 10.1;
 				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks";

iOS_SDK/OneSignalSDK/Source/OneSignalClient.m

@@ -29,11 +29,7 @@
 #import "UIApplicationDelegate+OneSignal.h"
 #import "ReattemptRequest.h"
 #import "OneSignal.h"
-
-#define REATTEMPT_DELAY 30.0
-#define REQUEST_TIMEOUT_REQUEST 60.0 //for most HTTP requests
-#define REQUEST_TIMEOUT_RESOURCE 100.0 //for loading a resource like an image
-#define MAX_ATTEMPT_COUNT 3
+#import "OneSignalCommonDefines.h"
 
 @interface OneSignal (OneSignalClientExtra)
 + (BOOL)shouldLogMissingPrivacyConsentErrorWithMethodName:(NSString *)methodName;
@@ -66,6 +62,14 @@ -(instancetype)init {
     return self;
 }
 
+- (NSError *)privacyConsentErrorWithRequestType:(NSString *)type {
+    return [NSError errorWithDomain:@"OneSignal Error" code:0 userInfo:@{@"error" : [NSString stringWithFormat: @"Attempted to perform an HTTP request (%@) before the user provided privacy consent.", type]}];
+}
+
+- (NSError *)genericTimedOutError {
+    return [NSError errorWithDomain:@"OneSignal Error" code:0 userInfo:@{@"error" : @"The request timed out"}];
+}
+
 - (void)executeSimultaneousRequests:(NSDictionary<NSString *, OneSignalRequest *> *)requests withSuccess:(OSMultipleSuccessBlock)successBlock onFailure:(OSMultipleFailureBlock)failureBlock {
     if (requests.allKeys.count == 0)
         return;
@@ -92,7 +96,15 @@ - (void)executeSimultaneousRequests:(NSDictionary<NSString *, OneSignalRequest *
             }];
         }
 
-        dispatch_group_wait(group, dispatch_time(DISPATCH_TIME_NOW, REQUEST_TIMEOUT_REQUEST * NSEC_PER_SEC));
+        // Will wait for up to (maxTimeout) seconds and will then give up and call
+        // the failure block if the request times out.
+        let timedOut = (bool)(0 != dispatch_group_wait(group, dispatch_time(DISPATCH_TIME_NOW, MAX_TIMEOUT)));
+        
+        // add a generic 'timed out' error if the request timed out
+        // and there are no other errors present.
+        if (timedOut && errors.allKeys.count == 0)
+            for (NSString *key in requests.allKeys)
+                errors[key] = [self genericTimedOutError];
 
         //requests should all be completed at this point
         dispatch_async(dispatch_get_main_queue(), ^{
@@ -109,9 +121,7 @@ - (void)executeRequest:(OneSignalRequest *)request onSuccess:(OSResultSuccessBlo
 
     if (request.method != GET && [OneSignal shouldLogMissingPrivacyConsentErrorWithMethodName:nil]) {
         if (failureBlock) {
-            failureBlock([NSError errorWithDomain:@"OneSignal Error" code:0
-                    userInfo:@{@"error" : [NSString stringWithFormat:
-                                    @"Attempted to perform an HTTP request (%@) before the user provided privacy consent.", NSStringFromClass(request.class)]}]);
+            failureBlock([self privacyConsentErrorWithRequestType:NSStringFromClass(request.class)]);
         }
 
         return;
@@ -198,7 +208,8 @@ - (BOOL)willReattemptRequest:(int)statusCode withRequest:(OneSignalRequest *)req
 
         if (async) {
             //retry again in 15 seconds
-            [OneSignal onesignal_Log:ONE_S_LL_DEBUG message:[NSString stringWithFormat:@"Re-scheduling request (%@) to be re-attempted in %i seconds due to failed HTTP request with status code %i", NSStringFromClass([request class]), (int)REATTEMPT_DELAY, (int)statusCode]];
+            [OneSignal onesignal_Log:ONE_S_LL_DEBUG message:[NSString stringWithFormat:@"Re-scheduling request (%@) to be re-attempted in %.3f seconds due to failed HTTP request with status code %i", NSStringFromClass([request class]), REATTEMPT_DELAY, (int)statusCode]];
+            NSLog(@"Delay: %f", REATTEMPT_DELAY);
 
             [OneSignalHelper performSelector:@selector(reattemptRequest:) onMainThreadOnObject:self withObject:reattempt afterDelay:REATTEMPT_DELAY];
         } else {

iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h

@@ -86,4 +86,27 @@
 // before registering the user anyways
 #define APNS_TIMEOUT 25.0
 
+#ifndef OS_TEST
+    // OneSignal API Client Defines
+    #define REATTEMPT_DELAY 30.0
+    #define REQUEST_TIMEOUT_REQUEST 120.0 //for most HTTP requests
+    #define REQUEST_TIMEOUT_RESOURCE 120.0 //for loading a resource like an image
+    #define MAX_ATTEMPT_COUNT 3
+
+    // Send tags batch delay
+    #define SEND_TAGS_DELAY 5.0
+#else
+    // Test defines for API Client
+    #define REATTEMPT_DELAY 0.04
+    #define REQUEST_TIMEOUT_REQUEST 0.2 //for most HTTP requests
+    #define REQUEST_TIMEOUT_RESOURCE 0.2 //for loading a resource like an image
+    #define MAX_ATTEMPT_COUNT 3
+
+    // Send tags batch delay
+    #define SEND_TAGS_DELAY 0.05
+#endif
+
+// A max timeout for a request, which might include multiple reattempts
+#define MAX_TIMEOUT ((REQUEST_TIMEOUT_REQUEST * MAX_ATTEMPT_COUNT) + (REATTEMPT_DELAY * MAX_ATTEMPT_COUNT)) * NSEC_PER_SEC
+
 #endif /* OneSignalCommonDefines_h */

iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.h

@@ -30,3 +30,8 @@
 @interface NSURLSessionOverrider : NSObject
 
 @end
+
+
+@interface MockNSURLSessionDataTask : NSURLSessionDataTask
+-(void)resume;
+@end

iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.m

@@ -26,14 +26,18 @@
  */
 
 #import "NSURLSessionOverrider.h"
-
+#import "OneSignalSelectorHelpers.h"
 #import "TestHelperFunctions.h"
+#import "OneSignalHelper.h"
+
+
 
 @implementation NSURLSessionOverrider
 
 + (void)load {
     // Swizzle an injected method defined in OneSignalHelper
     injectStaticSelector([NSURLSessionOverrider class], @selector(overrideDownloadItemAtURL:toFile:error:), [NSURLSession class], @selector(downloadItemAtURL:toFile:error:));
+    injectToProperClass(@selector(overrideDataTaskWithRequest:completionHandler:), @selector(dataTaskWithRequest:completionHandler:), @[], [NSURLSessionOverrider class], [NSURLSession class]);
 }
 
 // Override downloading of media attachment
@@ -52,4 +56,25 @@ + (NSString *)overrideDownloadItemAtURL:(NSURL*)url toFile:(NSString*)localPath
         return @"image/jpg";
 }
 
+- (NSURLSessionDataTask *)overrideDataTaskWithRequest:(NSURLRequest *)request completionHandler:(void (^)(NSData * _Nullable data, NSURLResponse * _Nullable response, NSError * _Nullable error))completionHandler {
+    
+    // mimics no active network connection
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.001 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
+        NSHTTPURLResponse *response = [[NSHTTPURLResponse alloc] initWithURL:request.URL statusCode:0 HTTPVersion:@"1.1" headerFields:@{}];
+        NSError *error = [NSError errorWithDomain:@"OneSignal Error" code:0 userInfo:@{@"error" : @"The user is not currently connected to the network."}];
+        NSLog(@"Calling completion handler");
+        completionHandler(nil, response, error);
+    });
+    
+    return [MockNSURLSessionDataTask new];
+}
+
+@end
+
+@implementation MockNSURLSessionDataTask
+
+-(void)resume {
+    //unimplemented
+}
+
 @end

iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.h

@@ -42,5 +42,6 @@
 +(NSString *)lastHTTPRequestType;
 +(void)setRequiresEmailAuth:(BOOL)required;
 +(BOOL)hasExecutedRequestOfType:(Class)type;
++ (void)disableExecuteRequestOverride:(BOOL)disable;
 @end
 

iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m

@@ -49,6 +49,7 @@ @implementation OneSignalClientOverrider
 static NSString *lastHTTPRequestType;
 static BOOL requiresEmailAuth = false;
 static NSMutableArray<NSString *> *executedRequestTypes;
+static BOOL disableOverride = false;
 
 + (void)load {
     serialMockMainLooper = dispatch_queue_create("com.onesignal.unittest", DISPATCH_QUEUE_SERIAL);
@@ -63,7 +64,14 @@ + (void)load {
     executedRequestTypes = [[NSMutableArray alloc] init];
 }
 
+// Calling this function twice results in reversing the swizzle
++ (void)disableExecuteRequestOverride:(BOOL)disable {
+    disableOverride = disable;
+}
+
 - (void)overrideExecuteSimultaneousRequests:(NSDictionary<NSString *, OneSignalRequest *> *)requests withSuccess:(OSMultipleSuccessBlock)successBlock onFailure:(OSMultipleFailureBlock)failureBlock {
+    if (disableOverride)
+        return [self overrideExecuteSimultaneousRequests:requests withSuccess:successBlock onFailure:failureBlock];
 
     dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
 
@@ -94,6 +102,10 @@ - (void)overrideExecuteSimultaneousRequests:(NSDictionary<NSString *, OneSignalR
 }
 
 - (void)overrideExecuteRequest:(OneSignalRequest *)request onSuccess:(OSResultSuccessBlock)successBlock onFailure:(OSFailureBlock)failureBlock {
+    
+    if (disableOverride)
+        return [self overrideExecuteRequest:request onSuccess:successBlock onFailure:failureBlock];
+    
     [executedRequestTypes addObject:NSStringFromClass([request class])];
 
     if (executeInstantaneously) {

iOS_SDK/OneSignalSDK/UnitTests/UnitTests.m

@@ -2051,4 +2051,46 @@ - (void)testRegistersAfterNoApnsResponse {
     XCTAssertTrue(observer->last.to.userId != nil);
 }
 
+/*
+ To prevent tests from generating actual HTTP requests, we swizzle
+ a method called executeRequest() in the OneSignalClient class
+ 
+ However, this test ensures that HTTP retry logic occurs correctly.
+ We have additionally swizzled NSURLSession to prevent real HTTP
+ requests from being generated.
+ 
+ TODO: Remove the OneSignalClientOverrider mock entirely and
+ instead swizzle NSURLSession
+ */
+- (void)testHTTPClientTimeout {
+    [OneSignal initWithLaunchOptions:nil appId:@"b2f7f966-d8cc-11e4-bed1-df8f05be55ba"
+            handleNotificationAction:nil
+                            settings:nil];
+    
+    [UnitTestCommonMethods runBackgroundThreads];
+    
+    // Switches from overriding OneSignalClient to using a
+    // swizzled NSURLSession instead. This results in NSURLSession
+    // mimicking a no-network connection state
+    [OneSignalClientOverrider disableExecuteRequestOverride:true];
+    
+    let expectation = [self expectationWithDescription:@"timeout_test"];
+    expectation.expectedFulfillmentCount = 1;
+    
+    [OneSignal sendTags:@{@"test_tag_key" : @"test_tag_value"} onSuccess:^(NSDictionary *result) {
+        XCTFail(@"Success should not be called");
+    } onFailure:^(NSError *error) {
+        [expectation fulfill];
+    }];
+    
+    [NSObjectOverrider runPendingSelectors];
+    [UnitTestCommonMethods runBackgroundThreads];
+    [NSObjectOverrider runPendingSelectors];
+    
+    [self waitForExpectations:@[expectation] timeout:10.0];
+    
+    // revert the swizzle back to the standard state for tests
+    [OneSignalClientOverrider disableExecuteRequestOverride:false];
+}
+
 @end