Comments (7)
@Lukasa @weissi I agree with what you said, I think I was too much worried about not being blocked after 1.0.0
, but it looks like your solution solves this @weissi. ๐
from async-http-client.
Reposting the relevant part of #105 (comment):
Make a breaking change before the 1.0.0 tag by requiring all delegate implementations to explicitly handle retries, this way a connection pool with retries can be added entirely transparently in the future.
Make no change to the current requirements of the delegate and add an an opt-in change in the future (by making a new type of delegate for retries or adding a default implementation): in this case, a connection pool could still be added, but no SemVer minor change could automatically add the retry mechanism for everyone except if the library user explicitly starts handling the retry.
I originally had a preference for the first option, but I also realise locking the API right now requires a really careful consideration of all possible cases to avoid being blocked when a connection pool is added in the future.
For instance, what are the failures the client should notify the delegate retry method about? There are the obvious ones like closed connections on idempotent HTTP requests, but there are multiple other cases: one thing we had previously discussed was error responses such as 429 Too Many Requests
or 503 Service Unavailable
that contain a Retry-After
header. In the case of non-idempotent HTTP requests, should they also be retried if there is a connection failure before the request has been fully sent?
I see there are a number of things to take into account, which makes me wonder if there is enough time to get this right or if it would be safer to add this as an opt-in option later?
I also thought about a solution that could be a mix of the two: for now, a new retryPreference
option would be added, as well as a didRetryRequest(...)
method on the delegate:
public struct RetryPreference {
/// All errors are directly transmitted to `didReceiveError(...)`
public static let noRetries: RetryPreference
/// If the client knows how to retry a certain error
/// it will do this and inform the delegate through
/// `didRetryRequest(...)`, otherwise it calls `didReceiveError(...)`
public static let automaticRetries: RetryPreference
...
}
public protocol HTTPClientResponseDelegate: AnyObject {
...
func didRetryRequest(task: HTTPClient.Task<Response>)
}
The kind of errors the client can retry itself coud evolve at any time, the actual error that caused the retry would be opaque to didRetryRequest
whose only goal would be to reset the delegate state.
When we want to allow retry customisation in the future, it would be easy to add a new RetryPreference
option, for instance:
public struct RetryPreference {
public func custom(handler: RetryHandler, mask: RetryHandlerMask) -> RetryPreference
...
}
public protocol RetryHandler {
func shouldRetryRequest(task: Task, for error: Error) -> EventLoopFuture<RetryDecision>
}
public enum RetryHandlerMask {
/// Transmit all errors to RetryHandler
case allErrors
/// Only transmit errors the client doesn't already know how to handle
case unsupportedErrors
}
from async-http-client.
I'm quite tempted by this approach, as it allows us to potentially shim a solution in by having an automatic-retry delegate that can be composed with a user-provided delegate in some way. In general, trying to do too much is likely to go badly for us.
Do you think RetryPreference
+ RetryHandler
+ RetryHandlerMask
would allow this composition?
from async-http-client.
My honest inclination is to not worry too much about this. If we find ourselves in a place where the delegate cannot be leveraged to solve this problem, we can break the API and go to v2 to solve the issue. We're running into a lot of roadblocks right now where we are trying to solve multiple things at once, combined with a desire to get a v1 out the door, and I am beginning to think that this is just causing us to be at risk of rushing APIs for no good reason.
from async-http-client.
@Lukasa / @adtrevor so what I don't quite understand (anymore?) is why we can't decide that later.
For now, we should say: No retries and once we're ready to introduce that, we just add two delegate methods:
shouldRetry(task: Task, error: Error, responseHead: HTTPServerResponseHead?) -> EventLoopFuture<Bool>
(called for the delegate to decide)willRetry(task: Task)
(called to notify the delegate that we've committed to retry)
Adding those two protocol methods with default implementations isn't breaking. For now, we could just imagine that we return false
in the (non-existant) shouldRetry
(and therefore the non-existant) willRetry
will never be called.
To make it easy for delegate providers to have sensible retry policies, we could offer standard implementations for those that do certain things.
We could even build in the user's retry policy and also hand that to the delegate on shouldRetry
. We could even make its signature shouldRetry(task: Task, error: Error, responseHead: HTTPServerResponseHead?, userRetryPreference: RetryPreference?) -> EventLoopFuture<Bool>
or so.
All this isn't breaking API and should work. Am I missing anything?
from async-http-client.
@Lukasa / @adtrevor so what I don't quite understand (anymore?) is why we can't decide that later.
For now, we should say: No retries and once we're ready to introduce that, we just add two delegate methods:
I think I'm agreeing with you: I'd rather do nothing and worry about the API consequences later. I care less about whether we think we can do it without breaking: it's ok to iterate on the API once we've shipped it, IMO. Swinging for perfection the first time out the gate is excessive.
That said, your approach does seem reasonable and it seems like it could well be added without too much difficulty.
from async-http-client.
Ok, let's hope and assume so :)
from async-http-client.
Related Issues (20)
- AHC holds onto its body stream writer even after the request is complete HOT 1
- Fatal error: SWIFT TASK CONTINUATION MISUSE: executeCancellable(_:deadline:logger:) tried to resume its continuation more than once, throwing HTTPClientError.deadlineExceeded! HOT 1
- Authenticate using certificate and passphrase like curl's `--cert` option HOT 2
- Fatal error: 'try!' expression unexpectedly raised an error: NIOCore.ChannelPipelineError.notFound HOT 1
- StreamClosed location NIOHTTP2/HTTP2StreamChannel.swift:827 HOT 6
- Many sockets in FinWait2 and CloseWait state HOT 7
- 'Transaction' is only available in iOS 13.0 or newer HOT 1
- It would be appreciated and useful to have a mechanism within AHC to handle Digest Auth HOT 1
- Privacy & security: Clarify what we log at which log level HOT 5
- Logging contains newlines (because it uses ByteBuffer.debugDescription which IMHO it shouldn't) HOT 11
- xrOS Support HOT 2
- Not supported features at `TLSConfiguration` HOT 2
- HTTPClient.shared (like URLSession.shared)? HOT 10
- Add `HTTPClient.Configuration.browserLike` HOT 7
- flaky test: TransactionTests.testCancelAsyncRequest
- HTTPClientRequest looks & feels like a value type but isn't HOT 10
- Option for `HTTPClientError.remoteConnectionClosed`s to be automatically retried HOT 1
- Shutdown does not close HTTP2 connection
- Linker Error on using AsyncHTTPClient while using Vapor inside XCode Project HOT 2
- crashed๏ผ free(): corrupted unsorted chunks #2617 HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. ๐๐๐
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google โค๏ธ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from async-http-client.