Skip to content
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

Retry Requests with HTTP Status 429 #4048

Merged
merged 52 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
7d3d883
keep track of retry counts
fire-at-will Jul 12, 2024
21dfb9e
immediately retry supported status codes
fire-at-will Jul 12, 2024
bdc6c03
execute retried request
fire-at-will Jul 12, 2024
e5db1b9
retry logic sans etags
fire-at-will Jul 12, 2024
5fe37d3
dont delay retries for first retry and no eTag header
fire-at-will Jul 12, 2024
6428262
delay conformance to Equatable
fire-at-will Jul 12, 2024
dfa171f
testing
fire-at-will Jul 12, 2024
6a74a22
linting
fire-at-will Jul 12, 2024
177ba49
stop retrying after a max count
fire-at-will Jul 12, 2024
9af7146
make baseBackoff & maxBackoff configurable
fire-at-will Jul 12, 2024
3117b86
add logging
fire-at-will Jul 12, 2024
85c809d
remove jitter from timeInterval delay
fire-at-will Jul 15, 2024
23e7a85
fix retry guard logic
fire-at-will Jul 15, 2024
664d80d
refactor guards + add log
fire-at-will Jul 15, 2024
aa41a7e
hard code retry times
fire-at-will Jul 15, 2024
e438e1b
write new tests
fire-at-will Jul 15, 2024
d9f4111
test fixes
fire-at-will Jul 15, 2024
e1c588f
optimizations from PR comments
fire-at-will Jul 16, 2024
5f5333d
update retry backoff time intervals
fire-at-will Jul 16, 2024
e0d09c9
add X-Retry-Count header + separate logging tests
fire-at-will Jul 16, 2024
36450e5
verify delays sent to operationdispatcher in test
fire-at-will Jul 16, 2024
3ae998a
allow for returning specific errors in HTTPClient + retry integration…
fire-at-will Jul 16, 2024
7e360e4
Generating new test snapshots for `retry-429s` - ios-15 (#4071)
RCGitBot Jul 16, 2024
fc3a871
Generating new test snapshots for `retry-429s` - ios-16 (#4067)
RCGitBot Jul 16, 2024
17bb1ad
Generating new test snapshots for `retry-429s` - ios-17 (#4070)
RCGitBot Jul 16, 2024
34a0015
Generating new test snapshots for `retry-429s` - ios-14 (#4068)
RCGitBot Jul 16, 2024
b034eaa
Generating new test snapshots for `retry-429s` - ios-13 (#4069)
RCGitBot Jul 16, 2024
0accbe2
Generating new test snapshots for `retry-429s` - macos (#4065)
RCGitBot Jul 16, 2024
0a65fef
Generating new test snapshots for `retry-429s` - watchos (#4064)
RCGitBot Jul 16, 2024
566598a
linting
fire-at-will Jul 16, 2024
b9c1f9b
refactor to avoid linting issue
fire-at-will Jul 16, 2024
71e8345
Merge branch 'retry-429s' of https://github.com/RevenueCat/purchases-…
fire-at-will Jul 16, 2024
2ab4382
lint
fire-at-will Jul 16, 2024
50d7453
dont run actual request if we force a failure
fire-at-will Jul 17, 2024
0c0775f
log message when we simulate a failure in HTTPClient
fire-at-will Jul 17, 2024
f31bd21
add more unit tests
fire-at-will Jul 17, 2024
5a3992e
replace integration test forcedServerErrors with OhHTTPStubs
fire-at-will Jul 17, 2024
5914732
remove pathRequestCounts
fire-at-will Jul 17, 2024
21399f5
add retry count to log message
fire-at-will Jul 17, 2024
7426811
validate stubbed request count in integration tests
fire-at-will Jul 17, 2024
619a361
fix test
fire-at-will Jul 17, 2024
3634049
linting
fire-at-will Jul 17, 2024
3c2fb0e
linting
fire-at-will Jul 17, 2024
2aefc4b
tweaks from code review
fire-at-will Jul 19, 2024
2dc4477
sort imports for linter
fire-at-will Jul 19, 2024
972f504
rename param to jitterableDelay
fire-at-will Jul 19, 2024
3b20186
Add Is-Retryable Header
fire-at-will Jul 22, 2024
da10908
rename to jitterableDelay
fire-at-will Jul 22, 2024
95848e4
linting
fire-at-will Jul 22, 2024
f886206
linting
fire-at-will Jul 22, 2024
ff28f23
server provided backoff interval validation
fire-at-will Jul 22, 2024
03fc420
treat Retry-After header value as seconds
fire-at-will Jul 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
8 changes: 8 additions & 0 deletions RevenueCat.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,8 @@
F5FCD3EA27DA0D0B003BDC04 /* PriceFormatterProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = F5FCD3E927DA0D0B003BDC04 /* PriceFormatterProvider.swift */; };
F5FCD3FC27DA2034003BDC04 /* PriceFormatterProviderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F5FCD3FB27DA2034003BDC04 /* PriceFormatterProviderTests.swift */; };
FD18ED4E2837F89200C5AA4F /* StoreKitWorkaroundsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = FD18ED4D2837F89200C5AA4F /* StoreKitWorkaroundsTests.swift */; };
FD43D2FC2C41864000077235 /* TimeInterval+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = FD43D2FA2C4185B700077235 /* TimeInterval+Extensions.swift */; };
FD43D2FE2C41867600077235 /* TimeInterval+ExtensionsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = FD43D2FD2C41867600077235 /* TimeInterval+ExtensionsTests.swift */; };
FD9F982D2BE28A7F0091A5BF /* MockNotificationCenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 351B515526D44B2300BD2BD7 /* MockNotificationCenter.swift */; };
FDAADFCB2BE2A5BF00BD1659 /* MockAllTransactionsProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = FDAADFCA2BE2A5BF00BD1659 /* MockAllTransactionsProvider.swift */; };
FDAADFCC2BE2A5BF00BD1659 /* MockAllTransactionsProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = FDAADFCA2BE2A5BF00BD1659 /* MockAllTransactionsProvider.swift */; };
Expand Down Expand Up @@ -1822,6 +1824,8 @@
F5FCD3E927DA0D0B003BDC04 /* PriceFormatterProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PriceFormatterProvider.swift; sourceTree = "<group>"; };
F5FCD3FB27DA2034003BDC04 /* PriceFormatterProviderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PriceFormatterProviderTests.swift; sourceTree = "<group>"; };
FD18ED4D2837F89200C5AA4F /* StoreKitWorkaroundsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StoreKitWorkaroundsTests.swift; sourceTree = "<group>"; };
FD43D2FA2C4185B700077235 /* TimeInterval+Extensions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TimeInterval+Extensions.swift"; sourceTree = "<group>"; };
FD43D2FD2C41867600077235 /* TimeInterval+ExtensionsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TimeInterval+ExtensionsTests.swift"; sourceTree = "<group>"; };
FDAADFCA2BE2A5BF00BD1659 /* MockAllTransactionsProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockAllTransactionsProvider.swift; sourceTree = "<group>"; };
FDAADFCE2BE2B84500BD1659 /* StoreKit2ObserverModePurchaseDetector.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StoreKit2ObserverModePurchaseDetector.swift; sourceTree = "<group>"; };
FDAADFD22BE2B99900BD1659 /* MockStoreKit2ObserverModePurchaseDetector.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockStoreKit2ObserverModePurchaseDetector.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1968,6 +1972,7 @@
57CD86D9291C1E2300768DE1 /* UserDefaults+Extensions.swift */,
4FC883802AA7A2BD00A3DE03 /* ProcessInfo+Extensions.swift */,
4F5C05BC2A43A21A00651C7D /* Locale+Extensions.swift */,
FD43D2FA2C4185B700077235 /* TimeInterval+Extensions.swift */,
);
path = FoundationExtensions;
sourceTree = "<group>";
Expand Down Expand Up @@ -2668,6 +2673,7 @@
5796A3BF27D7D64500653165 /* ResultExtensionsTests.swift */,
57A1772A276A726C0052D3A8 /* SetExtensionsTests.swift */,
57D56FC92853C005009E8E1E /* StringExtensionsTests.swift */,
FD43D2FD2C41867600077235 /* TimeInterval+ExtensionsTests.swift */,
);
path = FoundationExtensions;
sourceTree = "<group>";
Expand Down Expand Up @@ -4481,6 +4487,7 @@
578D79742936A36B0042E434 /* LoggerType.swift in Sources */,
B34605EB279A766C0031CA74 /* OperationQueue+Extensions.swift in Sources */,
57E6C2C72975AAE1001AFE98 /* FileReader.swift in Sources */,
FD43D2FC2C41864000077235 /* TimeInterval+Extensions.swift in Sources */,
4F7DBFBD2A1E986C00A2F511 /* StoreKit2TransactionFetcher.swift in Sources */,
5766AB4728401B8400FA6091 /* PackageType.swift in Sources */,
358C756C2C332BE800ECCA12 /* PreferredLocalesProvider.swift in Sources */,
Expand Down Expand Up @@ -4720,6 +4727,7 @@
57488B8B29CB756A0000EE7E /* ProductEntitlementMappingTests.swift in Sources */,
57ACB12428174B9F000DCC9F /* CustomerInfo+TestExtensions.swift in Sources */,
351B51B926D450E800BD2BD7 /* TransactionsFactoryTests.swift in Sources */,
FD43D2FE2C41867600077235 /* TimeInterval+ExtensionsTests.swift in Sources */,
351B51BB26D450E800BD2BD7 /* ProductRequestDataInitializationTests.swift in Sources */,
351B515426D44B0A00BD2BD7 /* MockStoreKit1Wrapper.swift in Sources */,
4F0BBAAC2A1D253D000E75AB /* OfflineCustomerInfoCreatorTests.swift in Sources */,
Expand Down
22 changes: 22 additions & 0 deletions Sources/FoundationExtensions/TimeInterval+Extensions.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
//
// Copyright RevenueCat Inc. All Rights Reserved.
//
// Licensed under the MIT License (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://opensource.org/licenses/MIT
//
// TimeInterval+Extensions.swift
//
// Created by Will Taylor on 7/12/24.

import Foundation

extension TimeInterval {

init(milliseconds: Double) {
self = milliseconds / 1000.0
}

}
18 changes: 18 additions & 0 deletions Sources/Logging/Strings/NetworkStrings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ enum NetworkStrings {
error: NetworkError,
metadata: HTTPClient.ResponseMetadata?)
case api_request_failed_status_code(HTTPStatusCode)
case api_request_queued_for_retry(httpMethod: String, path: String, backoffInterval: TimeInterval)
case api_request_failed_all_retries(httpMethod: String, path: String, retryCount: UInt)
case reusing_existing_request_for_operation(CacheableNetworkOperation.Type, String)
case enqueing_operation(CacheableNetworkOperation.Type, cacheKey: String)
case creating_json_error(error: String)
Expand All @@ -46,6 +48,7 @@ enum NetworkStrings {

#if DEBUG
case api_request_forcing_server_error(HTTPRequest)
case api_request_forcing_network_error(HTTPRequest, NetworkError)
case api_request_forcing_signature_failure(HTTPRequest)
case api_request_disabling_header_parameter_signature_verification(HTTPRequest)
#endif
Expand Down Expand Up @@ -131,10 +134,25 @@ extension NetworkStrings: LogMessage {
case let .request_handled_by_load_shedder(path):
return "Request was handled by load shedder: \(path.relativePath)"

case let .api_request_queued_for_retry(httpMethod, path, backoffInterval):
return "Queued request \(httpMethod) \(path) for retry in \(backoffInterval) seconds."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to log the retry number in this message as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this, will add it in!


case let .api_request_failed_all_retries(httpMethod, path, retryCount):
return "Request \(httpMethod) \(path) failed all \(retryCount) retries."

#if DEBUG
case let .api_request_forcing_server_error(request):
return "Returning fake HTTP 500 error for \(request.description)"

case let .api_request_forcing_network_error(request, networkError):
var simulatedHTTPStatusCode: HTTPStatusCode
if case .errorResponse(_, let statusCode, _) = networkError {
simulatedHTTPStatusCode = statusCode
} else {
simulatedHTTPStatusCode = .internalServerError
}
return "Returning fake HTTP \(simulatedHTTPStatusCode.rawValue) error for \(request.description)"

case let .api_request_forcing_signature_failure(request):
return "Returning fake signature verification failure for '\(request.description)'"

Expand Down
19 changes: 14 additions & 5 deletions Sources/Misc/Concurrency/OperationDispatcher.swift
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import Foundation
///
/// These delays prevent DDOS if a notification leads to many users opening an app at the same time,
/// by spreading asynchronous operations over time.
enum Delay {
enum JitterableDelay: Equatable {

case none
case `default`
case long
case timeInterval(TimeInterval)

static func `default`(forBackgroundedApp inBackground: Bool) -> Self {
return inBackground ? .default : .none
Expand Down Expand Up @@ -56,15 +57,19 @@ class OperationDispatcher {
Self.dispatchOnMainActor(block)
}

func dispatchOnWorkerThread(delay: Delay = .none, block: @escaping @Sendable () -> Void) {
func dispatchOnWorkerThread(delay: JitterableDelay = .none, block: @escaping @Sendable () -> Void) {
if delay.hasDelay {
self.workerQueue.asyncAfter(deadline: .now() + delay.random(), execute: block)
} else {
self.workerQueue.async(execute: block)
}
}

func dispatchOnWorkerThread(delay: Delay = .none, block: @escaping @Sendable () async -> Void) {
func dispatchOnWorkerThread(after timeInterval: TimeInterval, block: @escaping @Sendable () -> Void) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a small Josh nit but delay and after seem like very similar argument names and I would probably be confused by trying to figure out the difference between these 😅

Not a thing that has to change in the PR but just a thought 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you! The only difference is that delay: JitterableDelay doesn't let you specify the exact delay and includes a random jitter, while after timeInterval: TimeInterval allows you to specify the exact waiting time without a random jitter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can rename the param to jitterableDelay:

It might be slightly confusing on first glance but at least you'd hesitate long enough to actually think of which one to use

self.workerQueue.asyncAfter(deadline: .now() + timeInterval, execute: block)
}

func dispatchOnWorkerThread(delay: JitterableDelay = .none, block: @escaping @Sendable () async -> Void) {
Task.detached(priority: .background) {
if delay.hasDelay {
try? await Task.sleep(nanoseconds: DispatchTimeInterval(delay.random()).nanoseconds)
Expand All @@ -89,7 +94,7 @@ extension OperationDispatcher {
// MARK: -

/// Visible for testing
extension Delay {
extension JitterableDelay {

var hasDelay: Bool {
return self.maximum > 0
Expand All @@ -101,13 +106,15 @@ extension Delay {

}

private extension Delay {
private extension JitterableDelay {

var minimum: TimeInterval {
switch self {
case .none: return 0
case .`default`: return 0
case .long: return Self.maxJitter
case .timeInterval(let timeInterval):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't need the jitter, we might as well make another method in operation dispatcher that just takes an exact value as TimeInterval rather as an overload to the one we're using instead of passing in a min and max value that are the same

Copy link
Contributor Author

@fire-at-will fire-at-will Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the argument can be made either way. If we were to make that change, we'd then have two similar dispatch functions that happen after a period of time:

  • func dispatchOnWorkerThread(delay: Delay)
  • func dispatchOnWorkerThread(after timeInterval: TimeInterval)

When I read these function signatures, it isn't clear which one I should choose because the function signatures signify the same thing.

If we were to go through with this change, I'd recommend renaming Delay to something like JitterableDelay to indicate the difference that one of these APIs includes a jitter and the other doesn't. I'm down to make this change - any objections to that renaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I've gone ahead and renamed Delay to JitterableDelay and created the new func dispatchOnWorkerThread(after timeInterval: TimeInterval) function!

return max(timeInterval, 0)
Comment on lines +117 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when would this be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any instances where the timeInterval would be <0 in the scope of this PR, but this is just defensive programming in case someone ever tries to pass in a negative TimeInterval as a delay in the future

}
}

Expand All @@ -116,6 +123,8 @@ private extension Delay {
case .none: return 0
case .`default`: return Self.maxJitter
case .long: return Self.maxJitter * 2
case .timeInterval(let timeInterval):
return max(timeInterval, 0)
}
}

Expand Down
5 changes: 5 additions & 0 deletions Sources/Misc/DangerousSettings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,22 @@ import Foundation

#if DEBUG
let forceServerErrors: Bool
let forcedServerErrors: [String: [NetworkError]]?
let forceSignatureFailures: Bool
let disableHeaderSignatureVerification: Bool
let testReceiptIdentifier: String?

init(
enableReceiptFetchRetry: Bool = false,
forceServerErrors: Bool = false,
forcedServerErrors: [String: [NetworkError]]? = nil,
forceSignatureFailures: Bool = false,
disableHeaderSignatureVerification: Bool = false,
testReceiptIdentifier: String? = nil
) {
self.enableReceiptFetchRetry = enableReceiptFetchRetry
self.forceServerErrors = forceServerErrors
self.forcedServerErrors = forcedServerErrors
self.forceSignatureFailures = forceSignatureFailures
self.disableHeaderSignatureVerification = disableHeaderSignatureVerification
self.testReceiptIdentifier = testReceiptIdentifier
Expand Down Expand Up @@ -124,6 +127,8 @@ internal protocol InternalDangerousSettingsType: Sendable {
/// Whether `HTTPClient` will fake server errors
var forceServerErrors: Bool { get }

var forcedServerErrors: [String: [NetworkError]]? { get }
fire-at-will marked this conversation as resolved.
Show resolved Hide resolved

/// Whether `HTTPClient` will fake invalid signatures.
var forceSignatureFailures: Bool { get }

Expand Down
3 changes: 2 additions & 1 deletion Sources/Networking/Backend.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ class Backend {
eTagManager: eTagManager,
signing: Signing(apiKey: apiKey, clock: systemInfo.clock),
diagnosticsTracker: diagnosticsTracker,
requestTimeout: httpClientTimeout)
requestTimeout: httpClientTimeout,
operationDispatcher: OperationDispatcher.default)
let config = BackendConfiguration(httpClient: httpClient,
operationDispatcher: operationDispatcher,
operationQueue: QueueProvider.createBackendQueue(),
Expand Down
4 changes: 2 additions & 2 deletions Sources/Networking/BackendConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ extension BackendConfiguration {
/// Adds the `operation` to the `OperationQueue` (based on `CallbackCacheStatus`) potentially adding a random delay.
func addCacheableOperation<T: CacheableNetworkOperation>(
with factory: CacheableNetworkOperationFactory<T>,
delay: Delay,
delay: JitterableDelay,
cacheStatus: CallbackCacheStatus
) {
self.operationDispatcher.dispatchOnWorkerThread(delay: delay) {
Expand All @@ -64,7 +64,7 @@ extension BackendConfiguration {
/// Adds the `operation` to the diagnostics `OperationQueue` potentially adding a random delay.
func addDiagnosticsOperation(
_ operation: NetworkOperation,
delay: Delay = .long
delay: JitterableDelay = .long
) {
self.operationDispatcher.dispatchOnWorkerThread(delay: delay) {
self.diagnosticsQueue.addOperation(operation)
Expand Down
Loading