From 2eb9c3ccb2074e44d903a381f0a3ae5633001f94 Mon Sep 17 00:00:00 2001 From: Brad Hesse Date: Thu, 20 Sep 2018 16:41:02 -0700 Subject: [PATCH 1/2] Fix Request Re-Attempt Issue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • 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. --- iOS_SDK/OneSignalSDK/Source/OneSignalClient.m | 28 +++++++++++++------ .../Source/OneSignalCommonDefines.h | 9 ++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m b/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m index 10f2642f6..da459ac4e 100644 --- a/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m +++ b/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 *)requests withSuccess:(OSMultipleSuccessBlock)successBlock onFailure:(OSMultipleFailureBlock)failureBlock { if (requests.allKeys.count == 0) return; @@ -92,7 +96,15 @@ - (void)executeSimultaneousRequests:(NSDictionary Date: Fri, 21 Sep 2018 11:52:41 -0700 Subject: [PATCH 2/2] Add Testing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Adds testing to ensure that the reattempt logic works correctly --- .../OneSignal.xcodeproj/project.pbxproj | 5 +++ iOS_SDK/OneSignalSDK/Source/OneSignalClient.m | 3 +- .../Source/OneSignalCommonDefines.h | 24 ++++++++--- .../UnitTests/Shadows/NSURLSessionOverrider.h | 5 +++ .../UnitTests/Shadows/NSURLSessionOverrider.m | 27 +++++++++++- .../Shadows/OneSignalClientOverrider.h | 1 + .../Shadows/OneSignalClientOverrider.m | 12 ++++++ iOS_SDK/OneSignalSDK/UnitTests/UnitTests.m | 42 +++++++++++++++++++ 8 files changed, 112 insertions(+), 7 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj index b46bb879b..9398e2c14 100644 --- a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj +++ b/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"; diff --git a/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m b/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m index da459ac4e..ea0add11b 100644 --- a/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m +++ b/iOS_SDK/OneSignalSDK/Source/OneSignalClient.m @@ -208,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 { diff --git a/iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h b/iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h index 444744840..b2ecf8871 100644 --- a/iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h +++ b/iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h @@ -86,11 +86,25 @@ // before registering the user anyways #define APNS_TIMEOUT 25.0 -// 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 +#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 diff --git a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.h b/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.h index 313e168d8..a20fbe6bb 100644 --- a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.h +++ b/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.h @@ -30,3 +30,8 @@ @interface NSURLSessionOverrider : NSObject @end + + +@interface MockNSURLSessionDataTask : NSURLSessionDataTask +-(void)resume; +@end diff --git a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.m b/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.m index 6f3a52571..a470f0c93 100644 --- a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/NSURLSessionOverrider.m +++ b/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 diff --git a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.h b/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.h index daa132849..a7f3b596e 100644 --- a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.h +++ b/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 diff --git a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m b/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m index c63165130..9938bdc91 100644 --- a/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m +++ b/iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m @@ -49,6 +49,7 @@ @implementation OneSignalClientOverrider static NSString *lastHTTPRequestType; static BOOL requiresEmailAuth = false; static NSMutableArray *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 *)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:(NSDictionarylast.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