Report last connection error if request deadline is exceeded #601
Conversation
|
Your PR comment implies that if a user manually cancels we still report |
|
It's a bit hidden but we only report I guess if we don't want to report connection errors if the user cancels the request manually I will refactor the code a bit and make it more obvious what is going on. The code is currently a bit too generic and doesn't make it obvious that we actually only report connection errors if the cancellation reason was |
… level errors if the deadline was exceeded and not if the user cancels the request manually
|
@Lukasa I have removed |
| @@ -323,9 +323,10 @@ extension HTTPConnectionPool { | |||
|
|
|||
| mutating func cancelRequest(_ requestID: Request.ID) -> Action { | |||
| // 1. check requests in queue | |||
| if self.requests.remove(requestID) != nil { | |||
| if let request = self.requests.remove(requestID) { | |||
| let error = self.lastConnectFailure ?? HTTPClientError.cancelled | |||
There was a problem hiding this comment.
Please add a comment here, why we look into the lastConnectFailure.
There was a problem hiding this comment.
In theory we could check here if we are at max connections... In those cases we could issue a more explicit: deadlineExceededBecauseToQueueDepthToLong error.
There was a problem hiding this comment.
Not sure how useful this kind of error message would be because deadlineExceededBecauseToQueueDepthToLong may only be a symptom as other requests in front could not be scheduled because there was a connection failure.
| @@ -31,6 +31,8 @@ extension RequestBag { | |||
| fileprivate enum State { | |||
| case initialized | |||
| case queued(HTTPRequestScheduler) | |||
| /// if the deadline was exceeded while in the `.queued(_:)` state, we wait until the request pool fails the request with a potential more descriptive error message if a connection failure has occured while the request was queued. | |||
There was a problem hiding this comment.
NIT: can we wrap the comment here? I think normally I wrapped at about 100 characters.
| return .init( | ||
| request: .cancelRequestTimeout(requestID), | ||
| request: .failRequest(request, error, cancelTimeout: true), |
There was a problem hiding this comment.
Since, we remove .cancelRequestTimeout here and in the http2 state machine, I think we can remove this action completely.
| mutating func deadlineExceeded() -> DeadlineExceededAction { | ||
| switch self.state { | ||
| case .queued(let queuer): | ||
| self.state = .deadlineExceededWhileQueued |
There was a problem hiding this comment.
I would love a comment here, why we do this dance. Please also make explicit that we depend on the scheduler to fail this request. What happens, however if we think we are still queued, but we aren't actually? There might be a race here:
- request hit's request's deadline (thread a)
- connection becomes available (thread b)
- request is moved from pool queue to connection (thread b)
- request depends on pool to be cancelled (thread a), but pool ignores request since it is on the connection already
There was a problem hiding this comment.
For this we def. need a test case as well.
| case .finished(error: .some), .deadlineExceededWhileQueued: | ||
| return false |
There was a problem hiding this comment.
We'll need to fail the request here explicitly...
| case .queued(let queuer): | ||
| self.state = .deadlineExceededWhileQueued | ||
| return .cancelScheduler(queuer) | ||
| default: |
There was a problem hiding this comment.
Can we make all the other cases explicit here?
If the request deadline is exceeded, we should provide as much information as possible to let the user know the underlying reason why the deadline was exceeded. We should therefore report the last connection error instead of
HTTPClientError.deadlineExceededif the request was never executed because of a connection error.Modification
Do not fail the request in the
RequestBagif canceled. Instead, let the connection pool state machine fail the request as it has access to the last connection error.Result
Instead of
HTTPClientError.deadlineExceededwe now get a connection error if the request couldn't be scheduled because of a connection error.If a user manually cancels a request, we currently still just report
HTTPClientError.cancelled. However, we could now easily change the behavior and also report the last connection error if we decided to do so.async/await does not use
RequestBagbutTransactionand still has the old behaviour. I will apply similar changes toTransactionin a follow up PR.