figure out how delegates vs. retries

[weissi]

Once we have a channel pool (#105), we need to think about retries because there's a slightly higher chance that a request may fail.

We'll need to have the delegates take part in that retry dance. @adtrevor & @Lukasa discussed this here:

• Add a connection pool #105 (comment)
• Add a connection pool #105 (comment)

[PopFlamingo]

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 
}

[PopFlamingo]

@Lukasa in #105 (comment):

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?

[Lukasa]

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.

[weissi]

@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?

[Lukasa]

@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.

[PopFlamingo]

@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. 🙂

[weissi]

Ok, let's hope and assume so :)