hyperium / hyper

An HTTP library for Rust
https://hyper.rs
MIT License
14.58k stars 1.6k forks source link

Body-related errors of requests over HTTP/2 are swallowed by Hyper #2547

Open nox opened 3 years ago

nox commented 3 years ago

Errors emitted by the HttpBody impl sent as a request body through the HTTP/2 client are not returned to the caller.

https://github.com/hyperium/hyper/blob/8a05f8eec133793899c94dcbf1520eee3b91aa50/src/proto/h2/client.rs#L243-L247

I understand that this is because contrary to HTTP/1, the request body can be sent at the same as the response is received, so Hyper doesn't wait before returning the response, but that also means that the errors completely disappear and Hyper only notices something went wrong when trying to receive the response body, at which point it returns to the caller an error about the stream being reset.

Any ideas on how to improve that? Maybe we should implement an Extension that lets us await on a one-time channel to get the error?

seanmonstar commented 3 years ago

Are they completely swallowed? I think in all cases, they will bubble out the other sides. For instance, if the error comes from the h2 SendStream, it's likely from a stream or connection error. That will appear when polling for the response data. If the error is from the HttpBody, it will force sending a reset code, which the generic error will appear again when polling for the response data.

Well, I suppose if sending the body is the only thing left, you've already received the response and the full response body, then there'd be no way to see the error. Is that the situation you mean?

nox commented 3 years ago

By swallowed I mean we lose the exact reason that caused the error. Our system emits some errors on purpose and those should not be logged as they are completely expected behaviour, unfortunately they surface as a generic error as you said.

nox commented 3 years ago

What do you think of introducing a public extension in Hyper, something like:

type BoxError = Box<dyn Error + Send + Sync>;

fn request_body_poll_error_extension() -> (RequestBodyPollErrorExtension, RequestBodyPollErrorReceiver);

impl Future for RequestBodyPollErrorReceiver {
    type Output = Result<BoxError, Cancelled>;
}

In the implementation, RequestBodyPollErrorExtension and RequestBodyPollErrorReceiver would just be wrappers over Sender<BoxError> and Receiver<BoxError> from futures::channel::oneshot.

To use that extension, the user calls request_body_poll_error_extension and installs the returned RequestBodyPollErrorExtension in the request's extensions before passing it to the Hyper client.

The h1 support then completely disregards the extension, as it returns request body poll errors directly when sending the request before receiving the response.

On the h2 side, things get interesting.

First, we can patch this snippet to make pipe be a future that properly returns the error without silencing it:

https://github.com/hyperium/hyper/blob/8a05f8eec133793899c94dcbf1520eee3b91aa50/src/proto/h2/client.rs#L243-L247

Then, in this snippet, we make the eagerly poll fulfill the callback directly if an error has been returned already:

https://github.com/hyperium/hyper/blob/8a05f8eec133793899c94dcbf1520eee3b91aa50/src/proto/h2/client.rs#L251-L252

If the eager poll is pending, we look up the RequestBodyPollErrorExtension and we wrap pipe in a new future that will send any poll error on the onetime channel, there:

https://github.com/hyperium/hyper/blob/8a05f8eec133793899c94dcbf1520eee3b91aa50/src/proto/h2/client.rs#L259-L264

If the Hyper client returns Ok(response) to the user, the user is then free to check the RequestBodyPollErrorReceiver it kept, either on error while receiving the response body, or until the receiver returns Ok(error) (meaning the body was not sent successfully) or Err(Cancelled) (meaning the body was successfully sent).


That last part makes me wonder: how can a user even check that their request body was successfully sent over h2, if they are not polling that the response body was properly received?

nox commented 3 years ago

Given the last sentence of my previous comment, maybe the actual mechanism we need is some sort of "request body send observer", that we can await on until the request body was successfully grabbed and sent or an error was returned by poll_data.