-
Notifications
You must be signed in to change notification settings - Fork 72
fix: throttle retriable transport upload failures #770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 20 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
e0e7fbe
fix: throttle retriable transport upload failures
denischilik 42b6d59
fix: simplify transport retry throttling path
denischilik 3c29a84
fix: avoid throttling on offline and timeout errors
denischilik e88d1a8
fix: route throttle logging through MPLog
denischilik 2ec9e9b
test: add retry-after dictionary parser coverage
denischilik c691131
fix: simplify retry-after throttle parsing
denischilik 710af0a
fix: centralize retry-after calculation in network helper
denischilik 874935c
fix: throttle transport errors with internal backoff state
denischilik 6d90bdb
refactor: use a single throttle method with computed retry time
denischilik 689caa1
refactor: reuse shared mparticle instance in throttling
denischilik e2e21c7
fix method call
denischilik 6ad7a85
fix target for tests
denischilik 9258e61
fix: restore SDK timeout detection with shared connector constants
denischilik 6621ba7
fix: treat offline and timeout as retriable transport errors
denischilik 8564a52
fix: clamp transport retry schedule indexing
denischilik edd4b67
fix: reduce transport backoff schedule to short windows
denischilik 03cfc99
deduplicate constants
denischilik 1f84236
fix: tighten transport retries and align OneTrust selector
denischilik 8ab2f91
fix: use OneTrust vendorID selector for consent details
denischilik 9ef2c1c
fix: namespace retry-after NSDictionary extension methods
denischilik 5b13232
Merge branch 'main' into fix/retry-transport-detection
jamesnrokt File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import XCTest | ||
| internal import mParticle_Apple_SDK_Swift | ||
|
|
||
| final class MPTransportErrorDetectorTests: XCTestCase { | ||
| override func setUp() { | ||
| super.setUp() | ||
| MPTransportErrorDetector.resetTransportErrorCounter() | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsFalse_whenErrorIsNil() { | ||
| XCTAssertFalse(MPTransportErrorDetector.isRetriableTransportError(nil)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsFalse_whenNoConnectionCode() { | ||
| let error = NSError(domain: "any-domain", code: 1) | ||
| XCTAssertFalse(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsTrue_whenNSURLErrorIsRetriable() { | ||
| let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorCannotConnectToHost) | ||
| XCTAssertTrue(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsFalse_whenNSURLErrorIsNotRetriable() { | ||
| let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorCancelled) | ||
| XCTAssertFalse(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsTrue_whenNSURLErrorIsNotConnectedToInternet() { | ||
| let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorNotConnectedToInternet) | ||
| XCTAssertTrue(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsTrue_whenNSURLErrorIsTimedOut() { | ||
| let error = NSError(domain: NSURLErrorDomain, code: NSURLErrorTimedOut) | ||
| XCTAssertTrue(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsFalse_whenNSURLErrorIsAppTransportSecurityRequiresSecureConnection() { | ||
| let error = NSError( | ||
| domain: NSURLErrorDomain, | ||
| code: NSURLErrorAppTransportSecurityRequiresSecureConnection | ||
| ) | ||
| XCTAssertFalse(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsTrue_forMParticleTimeoutError() { | ||
| let error = NSError( | ||
| domain: MPTransportErrorDetector.semaphoreTimeoutErrorDomain() as String, | ||
| code: MPTransportErrorDetector.semaphoreTimeoutErrorCode().intValue | ||
| ) | ||
| XCTAssertTrue(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_isRetriableTransportError_returnsFalse_forUnknownError() { | ||
| let error = NSError(domain: "custom-domain", code: 42) | ||
| XCTAssertFalse(MPTransportErrorDetector.isRetriableTransportError(error)) | ||
| } | ||
|
|
||
| func test_calculateRetryTimeForTransportError_usesSmallValueForFirstError() { | ||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 5) | ||
| } | ||
|
|
||
| func test_calculateRetryTimeForTransportError_reachesMaxAtFiveErrors() { | ||
| for _ in 0..<4 { | ||
| _ = MPTransportErrorDetector.calculateRetryTimeForTransportError() | ||
| } | ||
|
|
||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 300) | ||
| } | ||
|
|
||
| func test_calculateRetryTimeForTransportError_usesExpectedBackoffSchedule() { | ||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 5) | ||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 15) | ||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 60) | ||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 120) | ||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 300) | ||
| } | ||
|
|
||
| func test_calculateRetryTimeForTransportError_staysAtMaxAfterThreshold() { | ||
| for _ in 0..<5 { | ||
| _ = MPTransportErrorDetector.calculateRetryTimeForTransportError() | ||
| } | ||
|
|
||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 300) | ||
| } | ||
|
|
||
| func test_resetTransportErrorCounter_resetsBackoff() { | ||
| _ = MPTransportErrorDetector.calculateRetryTimeForTransportError() | ||
| _ = MPTransportErrorDetector.calculateRetryTimeForTransportError() | ||
| MPTransportErrorDetector.resetTransportErrorCounter() | ||
|
|
||
| XCTAssertEqual(MPTransportErrorDetector.calculateRetryTimeForTransportError().doubleValue, 5) | ||
| } | ||
| } |
23 changes: 23 additions & 0 deletions
23
mParticle-Apple-SDK-Swift/Sources/Network/MPNetworkCommunicationHelper.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import Foundation | ||
|
|
||
| @objc public final class MPNetworkCommunicationHelper: NSObject { | ||
| @objc(calculateRetryTimeForHeaders:) | ||
| public static func calculateRetryTime(for headers: NSDictionary) -> NSNumber { | ||
| let retryAfterDate = headers.mp_retryDate() | ||
| let retryAfterSeconds = headers.mp_retrySeconds() | ||
| let defaultRetryAfter: Double = 7200 | ||
| let maxRetryAfter: Double = 86400 | ||
|
|
||
| if let retryAfterDate { | ||
| let now = Date() | ||
| let retryAfter = min(retryAfterDate.timeIntervalSince1970 - now.timeIntervalSince1970, maxRetryAfter) | ||
| return NSNumber(value: retryAfter > 0 ? retryAfter : defaultRetryAfter) | ||
| } | ||
|
|
||
| if let retryAfterSeconds { | ||
| return NSNumber(value: min(retryAfterSeconds.doubleValue, maxRetryAfter)) | ||
| } | ||
|
|
||
| return NSNumber(value: defaultRetryAfter) | ||
| } | ||
| } | ||
78 changes: 78 additions & 0 deletions
78
mParticle-Apple-SDK-Swift/Sources/Network/MPTransportErrorDetector.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import Foundation | ||
|
|
||
| @objc public class MPTransportErrorDetector: NSObject { | ||
| private static let maxRetryAfter: Double = 300 | ||
| private static let maxErrorCountBeforeMaxRetry = 5 | ||
| private static let retryAfterSchedule: [Double] = [5, 15, 60, 120, 300] | ||
| private static var consecutiveTransportErrorCount = 0 | ||
|
denischilik marked this conversation as resolved.
|
||
| private static let backoffQueue = DispatchQueue(label: "com.mparticle.transport-error-backoff") | ||
| private static let semaphoreTimeoutErrorDomainValue = "com.mparticle" | ||
| private static let semaphoreTimeoutErrorCodeValue = 0 | ||
|
denischilik marked this conversation as resolved.
|
||
|
|
||
| @objc(semaphoreTimeoutErrorDomain) | ||
| public static func semaphoreTimeoutErrorDomain() -> NSString { | ||
| semaphoreTimeoutErrorDomainValue as NSString | ||
| } | ||
|
|
||
| @objc(semaphoreTimeoutErrorCode) | ||
| public static func semaphoreTimeoutErrorCode() -> NSNumber { | ||
| NSNumber(value: semaphoreTimeoutErrorCodeValue) | ||
| } | ||
|
|
||
| @objc(isRetriableTransportError:) | ||
| public static func isRetriableTransportError(_ error: NSError?) -> Bool { | ||
| guard let error else { | ||
| return false | ||
| } | ||
|
|
||
| if error.domain == NSURLErrorDomain { | ||
| switch error.code { | ||
| case NSURLErrorCannotFindHost, | ||
| NSURLErrorCannotConnectToHost, | ||
| NSURLErrorNotConnectedToInternet, | ||
| NSURLErrorNetworkConnectionLost, | ||
| NSURLErrorDNSLookupFailed, | ||
| NSURLErrorTimedOut, | ||
| NSURLErrorCannotLoadFromNetwork, | ||
| NSURLErrorSecureConnectionFailed, | ||
|
denischilik marked this conversation as resolved.
|
||
| NSURLErrorInternationalRoamingOff, | ||
| NSURLErrorDataNotAllowed, | ||
| NSURLErrorCallIsActive: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
denischilik marked this conversation as resolved.
|
||
|
|
||
| return error.domain == semaphoreTimeoutErrorDomainValue | ||
| && error.code == semaphoreTimeoutErrorCodeValue | ||
| } | ||
|
|
||
| @objc(calculateRetryTimeForTransportError) | ||
| public static func calculateRetryTimeForTransportError() -> NSNumber { | ||
| return backoffQueue.sync { | ||
| consecutiveTransportErrorCount += 1 | ||
|
|
||
| if consecutiveTransportErrorCount >= maxErrorCountBeforeMaxRetry { | ||
| return NSNumber(value: maxRetryAfter) | ||
| } | ||
|
|
||
| guard !retryAfterSchedule.isEmpty else { | ||
| return NSNumber(value: maxRetryAfter) | ||
| } | ||
|
|
||
| let scheduleIndex = min( | ||
| max(0, consecutiveTransportErrorCount - 1), | ||
| retryAfterSchedule.count - 1 | ||
| ) | ||
| return NSNumber(value: retryAfterSchedule[scheduleIndex]) | ||
| } | ||
| } | ||
|
|
||
| @objc(resetTransportErrorCounter) | ||
| public static func resetTransportErrorCounter() { | ||
| backoffQueue.sync { | ||
| consecutiveTransportErrorCount = 0 | ||
| } | ||
| } | ||
| } | ||
26 changes: 26 additions & 0 deletions
26
mParticle-Apple-SDK-Swift/Sources/Network/NSDictionary+MPRetryAfter.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import Foundation | ||
|
|
||
| @objc public extension NSDictionary { | ||
| @objc(mp_retryDate) | ||
| func mp_retryDate() -> Date? { | ||
| guard let headerValue = self["Retry-After"] as? String else { | ||
| return nil | ||
| } | ||
|
|
||
| return MPDateFormatter.date(fromStringRFC1123: headerValue) | ||
| } | ||
|
|
||
| @objc(mp_retrySeconds) | ||
| func mp_retrySeconds() -> NSNumber? { | ||
| let headerValue = self["Retry-After"] | ||
|
|
||
| if let number = headerValue as? NSNumber { | ||
| return NSNumber(value: number.doubleValue) | ||
| } | ||
| if let string = headerValue as? String { | ||
| return Double(string).map { NSNumber(value: $0) } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
50 changes: 50 additions & 0 deletions
50
mParticle-Apple-SDK-Swift/Test/Utils/MPNetworkCommunicationHelperTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import XCTest | ||
| @testable import mParticle_Apple_SDK_Swift | ||
|
|
||
| final class MPNetworkCommunicationHelperTests: XCTestCase { | ||
| func testCalculateRetryTimeUsesNumberHeader() { | ||
| let headers: NSDictionary = ["Retry-After": NSNumber(value: 42)] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 42) | ||
| } | ||
|
|
||
| func testCalculateRetryTimeUsesStringSecondsHeader() { | ||
| let headers: NSDictionary = ["Retry-After": "123.5"] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 123.5) | ||
| } | ||
|
|
||
| func testCalculateRetryTimeFallsBackToDefaultForInvalidString() { | ||
| let headers: NSDictionary = ["Retry-After": "not-a-number"] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 7200) | ||
| } | ||
|
|
||
| func testCalculateRetryTimeFallsBackToDefaultWhenHeaderMissing() { | ||
| let headers: NSDictionary = ["Other-Header": "1"] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 7200) | ||
| } | ||
|
|
||
| func testCalculateRetryTimeClampsToMax() { | ||
| let headers: NSDictionary = ["Retry-After": NSNumber(value: 999999)] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 86400) | ||
| } | ||
|
|
||
| func testCalculateRetryTimeParsesDateHeader() { | ||
| let futureDate = Date().addingTimeInterval(300) | ||
| let retryAfterHeader = MPDateFormatter.string(fromDateRFC1123: futureDate) | ||
| let headers: NSDictionary = ["Retry-After": retryAfterHeader as Any] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 300, accuracy: 2.0) | ||
| } | ||
|
|
||
| func testCalculateRetryTimeUsesDefaultForPastDateHeader() { | ||
| let pastDate = Date().addingTimeInterval(-300) | ||
| let retryAfterHeader = MPDateFormatter.string(fromDateRFC1123: pastDate) | ||
| let headers: NSDictionary = ["Retry-After": retryAfterHeader as Any] | ||
|
|
||
| XCTAssertEqual(MPNetworkCommunicationHelper.calculateRetryTime(for: headers).doubleValue, 7200) | ||
| } | ||
| } |
40 changes: 40 additions & 0 deletions
40
mParticle-Apple-SDK-Swift/Test/Utils/NSDictionary+MPRetryAfterTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import XCTest | ||
| @testable import mParticle_Apple_SDK_Swift | ||
|
|
||
| final class NSDictionaryMPRetryAfterTests: XCTestCase { | ||
| func testRetryDateParsesRFC1123String() { | ||
| let headers: NSDictionary = ["Retry-After": "Wed, 21 Oct 2015 07:28:00 GMT"] | ||
|
|
||
| XCTAssertNotNil(headers.mp_retryDate()) | ||
| } | ||
|
|
||
| func testRetryDateReturnsNilForInvalidString() { | ||
| let headers: NSDictionary = ["Retry-After": "not-a-date"] | ||
|
|
||
| XCTAssertNil(headers.mp_retryDate()) | ||
| } | ||
|
|
||
| func testRetrySecondsReturnsNumberValue() { | ||
| let headers: NSDictionary = ["Retry-After": NSNumber(value: 42)] | ||
|
|
||
| XCTAssertEqual(headers.mp_retrySeconds()?.doubleValue, 42) | ||
| } | ||
|
|
||
| func testRetrySecondsParsesStringValue() { | ||
| let headers: NSDictionary = ["Retry-After": "123.5"] | ||
|
|
||
| XCTAssertEqual(headers.mp_retrySeconds()?.doubleValue, 123.5) | ||
| } | ||
|
|
||
| func testRetrySecondsReturnsNilForInvalidString() { | ||
| let headers: NSDictionary = ["Retry-After": "not-a-number"] | ||
|
|
||
| XCTAssertNil(headers.mp_retrySeconds()) | ||
| } | ||
|
|
||
| func testRetrySecondsReturnsNilWhenHeaderMissing() { | ||
| let headers: NSDictionary = ["Other-Header": "1"] | ||
|
|
||
| XCTAssertNil(headers.mp_retrySeconds()) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.