From 41eccfb96b620698a4aa6071de429ccb04acfdc8 Mon Sep 17 00:00:00 2001 From: jguz-pubnub Date: Tue, 19 Dec 2023 12:16:23 +0100 Subject: [PATCH] feat(auto-retry): fixes for AutomaticRetry --- .../Request/Operators/AutomaticRetry.swift | 81 +++--- .../Operators/AutomaticRetryTests.swift | 241 +++++++++--------- 2 files changed, 172 insertions(+), 150 deletions(-) diff --git a/Sources/PubNub/Networking/Request/Operators/AutomaticRetry.swift b/Sources/PubNub/Networking/Request/Operators/AutomaticRetry.swift index 6062018b..8b44ee61 100644 --- a/Sources/PubNub/Networking/Request/Operators/AutomaticRetry.swift +++ b/Sources/PubNub/Networking/Request/Operators/AutomaticRetry.swift @@ -26,16 +26,18 @@ public struct AutomaticRetry: RequestOperator, Hashable { policy: .defaultExponential, retryableURLErrorCodes: [.notConnectedToInternet] ) - + // The minimum value allowed between retries + static let minDelay: UInt = 2 + /// Provides the action taken when a retry is to be performed public enum ReconnectionPolicy: Hashable { - /// Exponential backoff with base/scale factor of 2, and a 300s max delay - public static let defaultExponential: ReconnectionPolicy = .exponential(base: 2, scale: 2, maxDelay: 300) - /// Linear reconnect every 3 seconds - public static let defaultLinear: ReconnectionPolicy = .linear(delay: 3) + /// Exponential backoff with base/scale factor of 2, and a 150s max delay + public static let defaultExponential: ReconnectionPolicy = .exponential(minDelay: minDelay, maxDelay: 150) + /// Linear reconnect every 2 seconds + public static let defaultLinear: ReconnectionPolicy = .linear(delay: Double(minDelay)) /// Reconnect with an exponential backoff - case exponential(base: UInt, scale: Double, maxDelay: UInt) + case exponential(minDelay: UInt, maxDelay: UInt) /// Attempt to reconnect every X seconds case linear(delay: Double) @@ -45,15 +47,15 @@ public struct AutomaticRetry: RequestOperator, Hashable { let randomDelay = Double.random(in: 0...1) switch self { - case let .exponential(base, scale, maxDelay): - return exponentialBackoffDelay(for: base, scale: scale, maxDelay: maxDelay, current: retryAttempt) + randomDelay + case let .exponential(minDelay, maxDelay): + return exponentialBackoffDelay(minDelay: minDelay, maxDelay: maxDelay, current: retryAttempt) + randomDelay case let .linear(delay): return delay + randomDelay } } - func exponentialBackoffDelay(for base: UInt, scale: Double, maxDelay: UInt, current retryCount: Int) -> Double { - return min(pow(Double(base), Double(retryCount)) * scale, Double(maxDelay)) + func exponentialBackoffDelay(minDelay: UInt, maxDelay: UInt, current retryCount: Int) -> Double { + return min(Double(maxDelay), Double(minDelay) * pow(2, Double(retryCount))) } } @@ -87,9 +89,9 @@ public struct AutomaticRetry: RequestOperator, Hashable { public let excluded: [AutomaticRetry.Endpoint] public init( - retryLimit: UInt = 2, + retryLimit: UInt = 6, policy: ReconnectionPolicy = .defaultExponential, - retryableHTTPStatusCodes: Set = [500], + retryableHTTPStatusCodes: Set = [500, 429], retryableURLErrorCodes: Set = AutomaticRetry.defaultRetryableURLErrorCodes, excluded endpoints: [AutomaticRetry.Endpoint] = [ .addChannelsToGroup, @@ -131,31 +133,42 @@ public struct AutomaticRetry: RequestOperator, Hashable { ] ) { switch policy { - case let .exponential(base, scale, max): - switch (true, true) { - case (base < 2, scale < 0): - PubNub.log.warn("The `exponential.base` must be a minimum of 2.") - PubNub.log.warn("The `exponential.scale` must be a positive value.") - self.policy = .exponential(base: 2, scale: 0, maxDelay: max) - case (base < 2, scale >= 0): - PubNub.log.warn("The `exponential.base` must be a minimum of 2.") - self.policy = .exponential(base: 2, scale: scale, maxDelay: max) - case (base >= 2, scale < 0): - PubNub.log.warn("The `exponential.scale` must be a positive value.") - self.policy = .exponential(base: base, scale: 0, maxDelay: max) - default: - self.policy = policy + case let .exponential(minDelay, maxDelay): + var finalMinDelay: UInt = minDelay + var finalMaxDelay: UInt = maxDelay + var finalRetryLimit: UInt = retryLimit + + if finalRetryLimit > 10 { + PubNub.log.warn("The `retryLimit` for exponential policy must be less than or equal 10") + finalRetryLimit = 10 + } + if finalMinDelay < Self.minDelay { + PubNub.log.warn("The `minDelay` must be a minimum of \(Self.minDelay)") + finalMinDelay = Self.minDelay + } + if finalMinDelay > finalMaxDelay { + PubNub.log.warn("The `minDelay` \"\(minDelay)\" must be greater or equal `maxDelay` \"\(maxDelay)\"") + finalMaxDelay = minDelay } + self.retryLimit = finalRetryLimit + self.policy = .exponential(minDelay: finalMinDelay, maxDelay: finalMaxDelay) + case let .linear(delay): - if delay < 0 { - PubNub.log.warn("The `linear.delay` must be a positive value.") - self.policy = .linear(delay: 0) - } else { - self.policy = policy + var finalRetryLimit = retryLimit + var finalDelay = delay + + if finalRetryLimit > 10 { + PubNub.log.warn("The `retryLimit` for linear policy must be less than or equal 10") + finalRetryLimit = 10 + } + if finalDelay < 0 || UInt(finalDelay) < Self.minDelay { + PubNub.log.warn("The `linear.delay` must be greater than or equal \(Self.minDelay).") + finalDelay = Double(Self.minDelay) } + self.retryLimit = finalRetryLimit + self.policy = .linear(delay: finalDelay) } - - self.retryLimit = retryLimit + self.retryableHTTPStatusCodes = retryableHTTPStatusCodes self.retryableURLErrorCodes = retryableURLErrorCodes self.excluded = endpoints @@ -184,7 +197,7 @@ public struct AutomaticRetry: RequestOperator, Hashable { func shouldRetry(response: HTTPURLResponse?, error: Error) -> Bool { if let statusCode = response?.statusCode { - return retryableHTTPStatusCodes.contains(statusCode) || statusCode == 429 + return retryableHTTPStatusCodes.contains(statusCode) } else if let errorCode = error.urlError?.code, retryableURLErrorCodes.contains(errorCode) { return true } else if let errorCode = error.pubNubError?.underlying?.urlError?.code, retryableURLErrorCodes.contains(errorCode) { diff --git a/Tests/PubNubTests/Networking/Operators/AutomaticRetryTests.swift b/Tests/PubNubTests/Networking/Operators/AutomaticRetryTests.swift index b8ed7ae0..b83da655 100644 --- a/Tests/PubNubTests/Networking/Operators/AutomaticRetryTests.swift +++ b/Tests/PubNubTests/Networking/Operators/AutomaticRetryTests.swift @@ -18,7 +18,7 @@ class AutomaticRetryTests: XCTestCase { func testReconnectionPolicy_DefaultLinearPolicy() { switch defaultLinearPolicy { case let .linear(delay): - XCTAssertEqual(delay, 3) + XCTAssertEqual(delay, 2) default: XCTFail("Default Linear Policy should only match to linear case") } @@ -26,10 +26,9 @@ class AutomaticRetryTests: XCTestCase { func testReconnectionPolicy_DefaultExponentialPolicy() { switch defaultExpoentialPolicy { - case let .exponential(base, scale, max): - XCTAssertEqual(base, 2) - XCTAssertEqual(scale, 2) - XCTAssertEqual(max, 300) + case let .exponential(minDelay, maxDelay): + XCTAssertEqual(minDelay, 2) + XCTAssertEqual(maxDelay, 150) default: XCTFail("Default Exponential Policy should only match to linear case") } @@ -39,77 +38,101 @@ class AutomaticRetryTests: XCTestCase { func testEquatable_Init_Valid_() { let testPolicy = AutomaticRetry.default - let policy = AutomaticRetry() + let automaticRetry = AutomaticRetry() - XCTAssertEqual(testPolicy, policy) + XCTAssertEqual(testPolicy, automaticRetry) } - func testEquatable_Init_Exponential_InvalidBase() { - let invalidBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(base: 0, scale: 3.0, maxDelay: 1) - let validBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(base: 2, scale: 3.0, maxDelay: 1) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: invalidBasePolicy, - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertNotEqual(testPolicy.policy, invalidBasePolicy) - XCTAssertEqual(testPolicy.policy, validBasePolicy) + func testEquatable_Init_Exponential_InvalidMinDelay() { + let invalidBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(minDelay: 0, maxDelay: 30) + let validBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(minDelay: 2, maxDelay: 30) + let automaticRetry = AutomaticRetry( + retryLimit: 2, + policy: invalidBasePolicy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertNotEqual(automaticRetry.policy, invalidBasePolicy) + XCTAssertEqual(automaticRetry.policy, validBasePolicy) } - - func testEquatable_Init_Exponential_InvalidScale() { - let invalidBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(base: 2, scale: -1.0, maxDelay: 1) - let validBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(base: 2, scale: 0.0, maxDelay: 1) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: invalidBasePolicy, - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertNotEqual(testPolicy.policy, invalidBasePolicy) - XCTAssertEqual(testPolicy.policy, validBasePolicy) + + func testEquatable_Init_Exponential_MinDelayGreaterThanMaxDelay() { + let invalidBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(minDelay: 10, maxDelay: 5) + let validBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(minDelay: 10, maxDelay: 10) + let automaticRetry = AutomaticRetry( + retryLimit: 2, + policy: invalidBasePolicy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertNotEqual(automaticRetry.policy, invalidBasePolicy) + XCTAssertEqual(automaticRetry.policy, validBasePolicy) } - - func testEquatable_Init_Exponential_InvalidBaseAndScale() { - let invalidBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(base: 0, scale: -1.0, maxDelay: 1) - let validBasePolicy = AutomaticRetry.ReconnectionPolicy.exponential(base: 2, scale: 0.0, maxDelay: 1) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: invalidBasePolicy, - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertNotEqual(testPolicy.policy, invalidBasePolicy) - XCTAssertEqual(testPolicy.policy, validBasePolicy) + + func testEquatable_Init_Exponential_TooHighRetryLimit() { + let policy = AutomaticRetry.ReconnectionPolicy.exponential(minDelay: 5, maxDelay: 60) + let automaticRetry = AutomaticRetry( + retryLimit: 12, + policy: policy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertEqual(automaticRetry.policy, policy) + XCTAssertEqual(automaticRetry.retryLimit, 10) } func testEquatable_Init_Linear_InvalidDelay() { let invalidBasePolicy = AutomaticRetry.ReconnectionPolicy.linear(delay: -1.0) - let validBasePolicy = AutomaticRetry.ReconnectionPolicy.linear(delay: 0.0) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: invalidBasePolicy, - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertNotEqual(testPolicy.policy, invalidBasePolicy) - XCTAssertEqual(testPolicy.policy, validBasePolicy) + let validBasePolicy = AutomaticRetry.ReconnectionPolicy.linear(delay: 2.0) + let automaticRetry = AutomaticRetry( + retryLimit: 2, + policy: invalidBasePolicy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertNotEqual(automaticRetry.policy, invalidBasePolicy) + XCTAssertEqual(automaticRetry.policy, validBasePolicy) + } + + func testEquatable_Init_Linear_TooHighRetryLimit() { + let policy = AutomaticRetry.ReconnectionPolicy.linear(delay: 3.0) + let automaticRetry = AutomaticRetry( + retryLimit: 12, + policy: policy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertEqual(automaticRetry.policy, policy) + XCTAssertEqual(automaticRetry.retryLimit, 10) } func testEquatable_Init_Linear_Valid() { - let validLinearPolicy = AutomaticRetry.ReconnectionPolicy.linear(delay: 1.0) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: validLinearPolicy, - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertEqual(testPolicy.policy, validLinearPolicy) + let validLinearPolicy = AutomaticRetry.ReconnectionPolicy.linear(delay: 3.0) + let automaticRetry = AutomaticRetry( + retryLimit: 2, + policy: validLinearPolicy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertEqual(automaticRetry.policy, validLinearPolicy) } func testEquatable_Init_Other() { let linearPolicy = AutomaticRetry.ReconnectionPolicy.linear(delay: 3.0) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: linearPolicy, - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertEqual(testPolicy.policy, linearPolicy) + let automaticRetry = AutomaticRetry( + retryLimit: 2, + policy: linearPolicy, + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertEqual(automaticRetry.policy, linearPolicy) } // MARK: - retry(:session:for:dueTo:completion:) @@ -136,89 +159,75 @@ class AutomaticRetryTests: XCTestCase { } let testStatusCode = 500 - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: .linear(delay: 3.0), - retryableHTTPStatusCodes: [testStatusCode], - retryableURLErrorCodes: []) - let testResponse = HTTPURLResponse(url: url, - statusCode: testStatusCode, - httpVersion: nil, - headerFields: [:]) - - XCTAssertTrue(testPolicy.shouldRetry(response: testResponse, - error: PubNubError(.unknown))) + let testPolicy = AutomaticRetry( + retryLimit: 2, + policy: .linear(delay: 3.0), + retryableHTTPStatusCodes: [testStatusCode], + retryableURLErrorCodes: [] + ) + let testResponse = HTTPURLResponse( + url: url, + statusCode: testStatusCode, + httpVersion: nil, + headerFields: [:] + ) + + XCTAssertTrue(testPolicy.shouldRetry(response: testResponse, error: PubNubError(.unknown))) } func testShouldRetry_True_ErrorCodeMatch() { let testURLErrorCode = URLError.Code.timedOut let testError = URLError(testURLErrorCode) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: .linear(delay: 3.0), - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: [testURLErrorCode]) - - XCTAssertTrue(testPolicy.shouldRetry(response: nil, - error: testError)) + let testPolicy = AutomaticRetry( + retryLimit: 2, + policy: .linear(delay: 3.0), + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [testURLErrorCode] + ) + + XCTAssertTrue(testPolicy.shouldRetry(response: nil, error: testError)) } func testShouldRetry_False() { let testError = URLError(.timedOut) - let testPolicy = AutomaticRetry(retryLimit: 2, - policy: .linear(delay: 3.0), - retryableHTTPStatusCodes: [], - retryableURLErrorCodes: []) - - XCTAssertFalse(testPolicy.shouldRetry(response: nil, - error: testError)) + let testPolicy = AutomaticRetry( + retryLimit: 2, + policy: .linear(delay: 3.0), + retryableHTTPStatusCodes: [], + retryableURLErrorCodes: [] + ) + + XCTAssertFalse(testPolicy.shouldRetry(response: nil, error: testError)) } // MARK: - exponentialBackoffDelay(for:scale:current:) func testExponentialBackoffDelay_DefaultScale() { let maxRetryCount = 5 - let scale = 2.0 - let base: UInt = 2 let maxDelay = UInt.max + // Usage of Range due to random delay (0...1) that's always added to the final value + let delayForRetry: [ClosedRange] = [2.0...3.0, 4.0...5.0, 8.0...9.0, 16.0...17.0, 32.0...33.0] - let delayForRetry: [ClosedRange] = [ - 4.0...5.0, - 8.0...9.0, - 16.0...17.0, - 32.0...33.0, - 64.0...65.0 - ] - - for count in 1 ... maxRetryCount { + for count in 0..] = [2.0...3.0, 3.0...4.0, 3.0...4.0, 3.0...4.0, 3.0...4.0] let maxRetryCount = 5 - let scale = 2.0 - let base: UInt = 2 - let maxDelay: UInt = 0 - - let delayForRetry: [ClosedRange] = [ - 0.0...1.0, - 0.0...1.0, - 0.0...1.0, - 0.0...1.0, - 0.0...1.0 - ] - - for count in 1 ... maxRetryCount { + + for count in 0..