diff --git a/CHANGELOG.md b/CHANGELOG.md index 730f21e..f57bc0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Change Log -## [2.1.9] - 2024-06-26 +## [2.2.0] - 2024-07-09 ### Changed - Fixed infinite retries for network errors diff --git a/README.md b/README.md index c832ec4..84d6f30 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ This will create initial files and folders including hidden​ `​.gitignore​ dependencies: [ // Dependencies declare other packages that this package depends on. ​// .package(url: /* package url */, from: "1.0.0"), - .​package​(​url​: "https://github.com/SiftScience/sift-ios.git"​, ​from​: "2.1.9"​) + .​package​(​url​: "https://github.com/SiftScience/sift-ios.git"​, ​from​: "2.2.0"​) ], ​targets​: [ ​// Targets are the basic building blocks of a package. A target can define a module or a test suite. diff --git a/Sift.podspec b/Sift.podspec index cc5c7c1..592eee4 100644 --- a/Sift.podspec +++ b/Sift.podspec @@ -1,6 +1,6 @@ Pod::Spec.new do |spec| spec.name = 'Sift' - spec.version = '2.1.9' + spec.version = '2.2.0' spec.authors = 'Sift Science' spec.license = { :type => 'MIT', diff --git a/Sift/Sift.m b/Sift/Sift.m index e6b9cd6..3e8881d 100644 --- a/Sift/Sift.m +++ b/Sift/Sift.m @@ -57,7 +57,7 @@ + (instancetype)sharedInstance { - (instancetype)initWithRootDirPath:(NSString *)rootDirPath { self = [super init]; if (self) { - _sdkVersion = @"v2.1.9"; + _sdkVersion = @"v2.2.0"; _rootDirPath = rootDirPath; diff --git a/Sift/SiftUploader.h b/Sift/SiftUploader.h index aa1e6d3..ea34552 100644 --- a/Sift/SiftUploader.h +++ b/Sift/SiftUploader.h @@ -10,7 +10,7 @@ NS_EXTENSION_UNAVAILABLE_IOS("SiftUploader is not supported for iOS extensions." - (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift; /** For testing. */ -- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase; +- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase networkRetryTimeout:(int64_t)networkRetryTimeout; /** Persist uploader state to disk. */ - (void)archive; diff --git a/Sift/SiftUploader.m b/Sift/SiftUploader.m index b353d9d..5461016 100644 --- a/Sift/SiftUploader.m +++ b/Sift/SiftUploader.m @@ -21,6 +21,8 @@ @implementation SiftUploader { NSMutableData *_responseBody; NSMutableArray *_batches; int _numRejects; + int _numNetworkRejects; + int64_t _maxNumNetworkRejects; int64_t _backoffBase; int64_t _backoff; NSString *_archivePath; @@ -31,6 +33,10 @@ @implementation SiftUploader { // Drop a batch if our backend has rejected it `SF_REJECT_LIMIT` times. static const int SF_REJECT_LIMIT = 3; +// Batch will be dropped if the network requests fail and the retry timeout period is reached. +static const int64_t SF_DEFAULT_NETWORK_RETRY_TIMEOUT = 10 * 60 * NSEC_PER_SEC; +static const int64_t SF_FALLBACK_NETWORK_MAX_RETRIES = 60; + static const int64_t SF_BACKOFF = NSEC_PER_SEC * 5; // Starting from 5 seconds. // Periodically check if we have unfinished batches. @@ -38,10 +44,13 @@ @implementation SiftUploader { static const int64_t SF_CHECK_UPLOAD_LEEWAY = 5 * NSEC_PER_SEC; - (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift { - return [self initWithArchivePath:archivePath sift:sift config:[NSURLSessionConfiguration defaultSessionConfiguration] backoffBase:SF_BACKOFF]; + return [self initWithArchivePath:archivePath sift:sift config:[NSURLSessionConfiguration defaultSessionConfiguration] backoffBase:SF_BACKOFF + networkRetryTimeout:SF_DEFAULT_NETWORK_RETRY_TIMEOUT]; } -- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase { +- (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift config:(NSURLSessionConfiguration *)config backoffBase:(int64_t)backoffBase + networkRetryTimeout:(int64_t)networkRetryTimeout { + self = [super init]; if (self) { _taskManager = [[TaskManager alloc] init]; @@ -52,7 +61,10 @@ - (instancetype)initWithArchivePath:(NSString *)archivePath sift:(Sift *)sift co _backoffBase = backoffBase; _backoff = backoffBase; _sift = sift; - + _maxNumNetworkRejects = networkRetryTimeout / backoffBase; + if (_maxNumNetworkRejects <= 0) { + _maxNumNetworkRejects = SF_FALLBACK_NETWORK_MAX_RETRIES; + } [self unarchive]; _source = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, _serial); @@ -96,10 +108,13 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp self->_responseBody = nil; BOOL success = NO; + BOOL networkError = NO; if (error) { SF_IMPORTANT(@"Could not complete upload due to %@", [error localizedDescription]); - self->_numRejects++; + self->_numNetworkRejects++; + networkError = YES; } else { + self->_numNetworkRejects = 0; NSInteger statusCode = [(NSHTTPURLResponse *)task.response statusCode]; SF_DEBUG(@"PUT %@ status %ld", task.response.URL, (long)statusCode); if (statusCode != 200) { @@ -123,10 +138,12 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp } } - if (self->_numRejects >= SF_REJECT_LIMIT) { + if (self->_numRejects >= SF_REJECT_LIMIT || + self->_numNetworkRejects >= self->_maxNumNetworkRejects) { NSLog(@"Drop a batch due to reject limit reached"); [self->_batches removeObjectAtIndex:0]; self->_numRejects = 0; + self->_numNetworkRejects = 0; self->_backoff = self->_backoffBase; } @@ -134,15 +151,21 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp if (success) { self->_backoff = self->_backoffBase; [self doUpload]; + } else if (networkError) { + [self retryUploadWithDelay:self->_backoffBase]; } else { - [self->_taskManager scheduleWithTask:^{ - [self doUpload]; - } queue:self->_serial delay:self->_backoff]; + [self retryUploadWithDelay:self->_backoff]; self->_backoff *= 2; } } queue:self->_serial]; } +- (void)retryUploadWithDelay:(NSTimeInterval)delay { + [self->_taskManager scheduleWithTask:^{ + [self doUpload]; + } queue:self->_serial delay:delay]; +} + - (void)doUpload { // Query the applicationState on the main thread, then proceed on the serial dispatch queue [self->_taskManager submitWithTask:^{ diff --git a/SiftTests/SiftUploaderNetworkBackoffTests.m b/SiftTests/SiftUploaderNetworkBackoffTests.m new file mode 100644 index 0000000..35fd5b3 --- /dev/null +++ b/SiftTests/SiftUploaderNetworkBackoffTests.m @@ -0,0 +1,74 @@ +// Copyright © 2024 Sift Science. All rights reserved. + +@import XCTest; + +#import "SiftEvent.h" +#import "Sift.h" +#import "Sift+Private.h" + +#import "SiftUploader.h" + +#import "SiftStubHttpProtocol.h" + +@interface SiftUploaderBackoffTests : XCTestCase + +@end + +@implementation SiftUploaderBackoffTests { + Sift *_sift; + SiftUploader *_uploader; +} + +- (void)setUp { + [super setUp]; + + _sift = [[Sift alloc] init]; // Don't call initWithRootDirPath. + _sift.accountId = @"account_id"; + _sift.beaconKey = @"beacon_key"; + _sift.userId = @"user_id"; + _sift.serverUrlFormat = @"mock+https://127.0.0.1/v3/accounts/%@/mobile_events"; + + // Set backoff = 5 seconds to test calculation of max retries for network errors + _uploader = [[SiftUploader alloc] initWithArchivePath:nil sift:_sift config:SFMakeStubConfig() backoffBase:5 * NSEC_PER_SEC networkRetryTimeout: 60 * NSEC_PER_SEC]; + + SFHttpStub *stub = [SFHttpStub sharedInstance]; + [stub.stubbedStatusCodes removeAllObjects]; + [stub.capturedRequests removeAllObjects]; +} + +- (void)tearDown { + [super tearDown]; +} + +- (void)testUploadMaxRetriesForNetworkErrorsWithBaseBackoff { + SFHttpStub *stub = [SFHttpStub sharedInstance]; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for upload tasks completion"]; + stub.completionHandler = ^{ + [expectation fulfill]; + }; + + // add stubs to the queue + int rejectedLimitCount = 100; + for (int i = 0; i < rejectedLimitCount; i++) { + // Simulate a network error response (status code 1 hardcoded for network errors). + [stub.stubbedStatusCodes addObject:@1]; + } + + NSArray *events = @[[SiftEvent eventWithType:nil path:@"path" fields:nil]]; + [_uploader upload:events]; + + XCTWaiter *waiter = [[XCTWaiter alloc] init]; + // max network retries should not exceed 1 min + XCTWaiterResult result = [waiter waitForExpectations:@[expectation] timeout:61.0]; + + // The waiter should time out because it's waiting for the final call (rejectedLimitCount = 100) to be made. + // However, the call is not made because the system stops retrying after max retries for network failed attempts (60 / backoffBase). + XCTAssertEqual(result, XCTWaiterResultTimedOut); + // Assert that no more than 12 (60 / backoffBase) calls were made + XCTAssertEqual(stub.capturedRequests.count, 60 / 5); + // 88 (initial stubs in the queue minus 12 captured requests) stubs should still be in the queue + XCTAssertEqual(stub.stubbedStatusCodes.count, rejectedLimitCount - 60/5); +} + +@end diff --git a/SiftTests/SiftUploaderTests.m b/SiftTests/SiftUploaderTests.m index f04c2f4..9980665 100644 --- a/SiftTests/SiftUploaderTests.m +++ b/SiftTests/SiftUploaderTests.m @@ -29,7 +29,7 @@ - (void)setUp { _sift.serverUrlFormat = @"mock+https://127.0.0.1/v3/accounts/%@/mobile_events"; // Disable exponential backoff with baseoffBase = 0. - _uploader = [[SiftUploader alloc] initWithArchivePath:nil sift:_sift config:SFMakeStubConfig() backoffBase:0]; + _uploader = [[SiftUploader alloc] initWithArchivePath:nil sift:_sift config:SFMakeStubConfig() backoffBase:0 networkRetryTimeout: 60 * NSEC_PER_SEC]; SFHttpStub *stub = [SFHttpStub sharedInstance]; [stub.stubbedStatusCodes removeAllObjects]; @@ -107,7 +107,7 @@ - (void)testUploadWithNetworkError { [expectation fulfill]; }; - int rejectedLimitCount = 3; + int rejectedLimitCount = 60; for (int i = 0; i < rejectedLimitCount; i++) { // Simulate a network error response (status code 1 hardcoded for network errors). [stub.stubbedStatusCodes addObject:@1]; @@ -130,7 +130,7 @@ - (void)testUploadWithNetworkErrorAndTimeout { [expectation fulfill]; }; - int rejectedLimitCount = 4; + int rejectedLimitCount = 61; for (int i = 0; i < rejectedLimitCount; i++) { // Simulate a network error response (status code 1 hardcoded for network errors). [stub.stubbedStatusCodes addObject:@1]; @@ -142,13 +142,76 @@ - (void)testUploadWithNetworkErrorAndTimeout { XCTWaiter *waiter = [[XCTWaiter alloc] init]; XCTWaiterResult result = [waiter waitForExpectations:@[expectation] timeout:3.0]; - // The waiter should time out because it's waiting for the final call (call 4) to be made. - // However, the call is not made because the system stops retrying after three failed attempts (SF_REJECT_LIMIT). + // The waiter should time out because it's waiting for the final call (call 61) to be made. + // However, the call is not made because the system stops retrying after 60 failed attempts (SF_DEFAULT_NETWORK_MAX_RETRIES). XCTAssertEqual(result, XCTWaiterResultTimedOut); - // Assert that no more than 3(SF_REJECT_LIMIT) calls were made - XCTAssertEqual(stub.capturedRequests.count, 3); + // Assert that no more than 60(SF_DEFAULT_NETWORK_MAX_RETRIES) calls were made + XCTAssertEqual(stub.capturedRequests.count, 60); // 1 stub should still be in the queue XCTAssertEqual(stub.stubbedStatusCodes.count, 1); } +- (void)testUploadNetworkErrorsMaxRetriesNotReached { + SFHttpStub *stub = [SFHttpStub sharedInstance]; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for upload tasks completion"]; + stub.completionHandler = ^{ + [expectation fulfill]; + }; + + int rejectedLimitCount = 59; + for (int i = 0; i < rejectedLimitCount; i++) { + // Simulate a network error response (status code 1 hardcoded for network errors). + [stub.stubbedStatusCodes addObject:@1]; + } + + int successfullRequests = 1; + for (int i = 0; i < successfullRequests; i++) { + [stub.stubbedStatusCodes addObject:@200]; + } + + NSArray *events = @[[SiftEvent eventWithType:nil path:@"path" fields:nil]]; + [_uploader upload:events]; + + [self waitForExpectationsWithTimeout:3.0 handler:nil]; + + XCTAssertEqual(stub.capturedRequests.count, rejectedLimitCount + successfullRequests); + XCTAssertEqual(stub.stubbedStatusCodes.count, 0); +} + +- (void)testUploadNetworkAndHttpErrorsMaxRetriesNotReached { + SFHttpStub *stub = [SFHttpStub sharedInstance]; + + XCTestExpectation *expectation = [self expectationWithDescription:@"Wait for upload tasks completion"]; + stub.completionHandler = ^{ + [expectation fulfill]; + }; + + int rejectedLimitCount = 59; + for (int i = 0; i < rejectedLimitCount; i++) { + // Simulate a network error response (status code 1 hardcoded for network errors). + [stub.stubbedStatusCodes addObject:@1]; + } + + int rejectedHttpErrorLimitCount = 2; + for (int i = 0; i < rejectedHttpErrorLimitCount; i++) { + [stub.stubbedStatusCodes addObject:@500]; + } + + int successfullRequests = 1; + for (int i = 0; i < successfullRequests; i++) { + [stub.stubbedStatusCodes addObject:@200]; + } + + NSArray *events = @[[SiftEvent eventWithType:nil path:@"path" fields:nil]]; + [_uploader upload:events]; + + [self waitForExpectationsWithTimeout:3.0 handler:nil]; + + + XCTAssertEqual(stub.capturedRequests.count, rejectedLimitCount + successfullRequests + rejectedHttpErrorLimitCount); + // verify that max retries for network and http errors are not reached + XCTAssertEqual(stub.stubbedStatusCodes.count, 0); +} + @end