-
Notifications
You must be signed in to change notification settings - Fork 24
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
[Devin] Add RetryInterceptor #52
base: main
Are you sure you want to change the base?
Changes from 2 commits
b375420
a0719a2
721b986
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ extension Provider { | |
public convenience init(baseURL: String, | ||
urlSession: URLSession = .shared, | ||
modifiers: [RequestModifier] = [], | ||
interceptors: [Interceptor] = []) { | ||
interceptors: [Interceptor] = [RetryInterceptor()]) { | ||
self.init(baseURL: baseURL, http: urlSession, modifiers: modifiers, interceptors: interceptors) | ||
} | ||
} | ||
|
@@ -58,13 +58,13 @@ extension Response { | |
private struct _Response: Response { | ||
let urlRequest: URLRequest | ||
let urlResponse: URLResponse? | ||
|
||
var request: Request? { urlRequest } | ||
let error: Error? | ||
let body: Data? | ||
let headers: [String: String]? | ||
var statusCode: Int? { (urlResponse as? HTTPURLResponse)?.statusCode } | ||
|
||
init(request: URLRequest, response: URLResponse?, error: Error?, body: Data?) { | ||
self.urlRequest = request | ||
self.urlResponse = response | ||
|
@@ -76,7 +76,7 @@ private struct _Response: Response { | |
guard let key = key as? String, let value = value as? String else { | ||
return nil | ||
} | ||
|
||
return (key, value) | ||
} | ||
if let headerPairs { | ||
|
@@ -111,3 +111,50 @@ extension URLRequest: Request { | |
set { allHTTPHeaderFields = newValue } | ||
} | ||
} | ||
|
||
// MARK: Retry Interceptor | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make these triple slash instead of slash asterisk comments |
||
A `RetryInterceptor` that conforms to the `Interceptor` protocol. | ||
This interceptor will retry a request up to a specified number of times | ||
if it fails with a status code in the range 500-599. | ||
|
||
- Parameters: | ||
- maxRetryCount: The maximum number of retry attempts. Default is 3. | ||
- initialBackoff: The initial backoff time in seconds before the first retry. Default is 2 seconds. | ||
- exponentialBackoff: The multiplier to apply to the backoff time after each retry. Default is 2. | ||
*/ | ||
public struct RetryInterceptor: Interceptor { | ||
private let maxRetryCount: Int | ||
private let initialBackoff: UInt64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make this int not int64 |
||
private let exponentialBackoff: UInt64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Int not int64 |
||
|
||
public init(maxRetryCount: Int = 3, initialBackoff: UInt64 = 2, exponentialBackoff: UInt64 = 2) { | ||
self.maxRetryCount = maxRetryCount | ||
self.initialBackoff = initialBackoff | ||
self.exponentialBackoff = exponentialBackoff | ||
} | ||
|
||
public func intercept(req: Request, next: Next) async throws -> Response { | ||
var response: Response | ||
var retryCount = 0 | ||
var backoff = initialBackoff | ||
|
||
repeat { | ||
do { | ||
response = try await next(req) | ||
if let statusCode = response.statusCode, (500...599).contains(statusCode) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably if we implement just retry strategy - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep totally agree - this PR was actually made by a bot. The GitHub copilot version (other PR) does what you suggest and I think is a much better option. |
||
retryCount += 1 | ||
try await Task.sleep(nanoseconds: backoff * 1_000_000_000) // Backoff in seconds | ||
backoff *= exponentialBackoff | ||
} else { | ||
return response | ||
} | ||
} catch { | ||
throw error | ||
} | ||
} while retryCount < maxRetryCount | ||
|
||
return response | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,76 +6,76 @@ final class APITests: XCTestCase { | |
func testApiEndpointReturnsNilForOptionalReturnType_forNilBody() async throws { | ||
// Arrange | ||
let sut = _PeopleAPI(provider: .init(baseURL: "", http: _HTTPServiceMock(responseType: .nil))) | ||
|
||
// Act | ||
let person = try await sut.getOptional() | ||
|
||
// Assert | ||
XCTAssertNil(person) | ||
} | ||
|
||
func testApiEndpointThrowsForNonOptionalReturnType_forNilBody() async throws { | ||
// Arrange | ||
let sut = _PeopleAPI(provider: .init(baseURL: "", http: _HTTPServiceMock(responseType: .nil))) | ||
|
||
// Act | ||
let expectation = expectation(description: "The endpoint with the non-optional return type should throw an error for an invalid body.") | ||
let expectation = self.expectation(description: "The endpoint with the non-optional return type should throw an error for an invalid body.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove |
||
do { | ||
let _ = try await sut.get() | ||
} catch { | ||
expectation.fulfill() | ||
} | ||
|
||
// Assert | ||
await fulfillment(of: [expectation], timeout: 1) | ||
wait(for: [expectation], timeout: 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert |
||
} | ||
|
||
func testApiEndpointReturnsNilForOptionalReturnType_forEmptyBody() async throws { | ||
// Arrange | ||
let sut = _PeopleAPI(provider: .init(baseURL: "", http: _HTTPServiceMock(responseType: .empty))) | ||
|
||
// Act | ||
let person = try await sut.getOptional() | ||
|
||
// Assert | ||
XCTAssertNil(person) | ||
} | ||
|
||
func testApiEndpointThrowsForNonOptionalReturnType_forEmptyBody() async throws { | ||
// Arrange | ||
let sut = _PeopleAPI(provider: .init(baseURL: "", http: _HTTPServiceMock(responseType: .empty))) | ||
|
||
// Act | ||
let expectation = expectation(description: "The endpoint with the non-optional return type should throw an error for an invalid body.") | ||
let expectation = self.expectation(description: "The endpoint with the non-optional return type should throw an error for an invalid body.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove |
||
do { | ||
let _ = try await sut.get() | ||
} catch { | ||
expectation.fulfill() | ||
} | ||
|
||
// Assert | ||
await fulfillment(of: [expectation], timeout: 1) | ||
wait(for: [expectation], timeout: 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revert |
||
} | ||
|
||
func testApiEndpointReturnsValidObjectForOptionalReturnType() async throws { | ||
// Arrange | ||
let sut = _PeopleAPI(provider: .init(baseURL: "", http: _HTTPServiceMock(responseType: .person))) | ||
|
||
// Act | ||
let person = try await sut.getOptional() | ||
|
||
// Assert | ||
XCTAssertNotNil(person) | ||
XCTAssertEqual(person?.name, "Petru") | ||
} | ||
|
||
func testApiEndpointReturnsValidObjectForNonOptionalReturnType() async throws { | ||
// Arrange | ||
let sut = _PeopleAPI(provider: .init(baseURL: "", http: _HTTPServiceMock(responseType: .person))) | ||
|
||
// Act | ||
let person = try await sut.get() | ||
|
||
// Assert | ||
XCTAssertNotNil(person) | ||
XCTAssertEqual(person.name, "Petru") | ||
|
@@ -84,10 +84,10 @@ final class APITests: XCTestCase { | |
|
||
@API() | ||
fileprivate protocol _People { | ||
|
||
@GET("") | ||
func getOptional() async throws -> _Person? | ||
|
||
@GET("") | ||
func get() async throws -> _Person | ||
} | ||
|
@@ -97,38 +97,38 @@ fileprivate struct _Person: Decodable { | |
} | ||
|
||
fileprivate class _HTTPServiceMock: HTTPService { | ||
|
||
enum ResponseType { | ||
case `nil` | ||
case empty | ||
case person | ||
|
||
var value: String? { | ||
switch self { | ||
case .nil: | ||
nil | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove all these returns |
||
case .empty: | ||
"" | ||
return "" | ||
case .person: | ||
"{\"name\": \"Petru\"}" | ||
return "{\"name\": \"Petru\"}" | ||
} | ||
} | ||
} | ||
|
||
private let _responseType: ResponseType | ||
|
||
init(responseType: ResponseType) { | ||
_responseType = responseType | ||
} | ||
|
||
func build(method: String, url: URL, headers: [String : String], body: Data?) -> Request { | ||
_Request(method: "", headers: [:]) | ||
} | ||
|
||
func request(_ req: PapyrusCore.Request) async -> PapyrusCore.Response { | ||
_Response(body: _responseType.value?.data(using: .utf8), statusCode: 200) | ||
} | ||
|
||
func request(_ req: PapyrusCore.Request, completionHandler: @escaping (PapyrusCore.Response) -> Void) { | ||
completionHandler(_Response(body: "".data(using: .utf8))) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove