Crash fix: HTTP2ClientRequestHandler can deal with failing writes #558
Conversation
fabianfett
force-pushed
the
ff-crash-in-http-client-request-handler
branch
from
308fdd0 to
ed98b97
Compare
Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift
Show resolved
Hide resolved
| // The above response head forward might lead the request to mark itself as | ||
| // cancelled, which in turn might pop the request of the handler. For this reason we | ||
| // must check if the request is still present here. | ||
| request.pauseRequestBodyStream() |
There was a problem hiding this comment.
This all seems very fragile. Each action is very tightly coupled to the state machine to check its own internal state, and some of these methods require non-local reasoning (e.g. calling a method on a request which reaches up and nils that request variable in its owner object).
It's perhaps worth questioning: with the tight coupling, is it worth dispatching all of these operations with a single enum? Perhaps each case should be an individual method, invoked directly with the unwrapped request value. The cancellation stuff is also not ideal but I don't have a concrete suggestion at this time.
Certainly the crash fix is important and I'm not suggesting it be delayed for refactoring, but it's maybe worth making a note to take another look at it later, and see if it can be made more robust.
There was a problem hiding this comment.
Very much agree! I guess that's the big learning from this state machine.
Same fix for HTTP/1 that landed for HTTP/2 in swift-server#558. ### Motivation `HTTP1ClientChannelHandler` currently does not tolerate immediate write errors. ### Changes Make `HTTP1ClientChannelHandler` resilient to failing writes. ### Result Less crashes in AHC HTTP/1.
Same fix for HTTP/1 that landed for HTTP/2 in #558. ### Motivation `HTTP1ClientChannelHandler` currently does not tolerate immediate write errors. ### Changes Make `HTTP1ClientChannelHandler` resilient to failing writes. ### Result Less crashes in AHC HTTP/1.
Motivation
AHC can currently not deal with direct failures when writing requests.
Changes
HTTP2ClientRequestHandlerresilient to failing writesResult
Less crashes in AHC HTTP/2.