search

hyperium / hyper

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

Client: handle `RST_STREAM` with `NO_ERROR` set for the reason #2872

Closed abonander closed 1 year ago

abonander commented 2 years ago

Version

hyper = "0.14.18"
h2 = "0.3.13"

Platform

> uname -a
Linux <REDACTED> 5.17.5-76051705-generic #202204271406~1651504840~22.04~63e51bd SMP PREEMPT Mon May 2 15: x86_64 x86_64 x86_64 GNU/Linux

Description I've found that Google Cloud Storage's API can respond with HTTP/2 RST_STREAM frame with NO_ERROR set for the reason, which appears to mean "stop sending the request body and read my response" according to https://datatracker.ietf.org/doc/html/rfc7540#section-8.1

A server can send a complete response prior to the client sending an entire request if the response does not depend on any portion of the request that has not been sent and received. When this is true, a server MAY request that the client abort transmission of a request without error by sending a RST_STREAM with an error code of NO_ERROR after sending a complete response (i.e., a frame with the END_STREAM flag). Clients MUST NOT discard responses as a result of receiving such a RST_STREAM, though clients can always discard responses at their discretion for other reasons.

I believe this is happening in response to a PutObject request when the bucket is being rate limited for writes. The server is trying to tell the client to stop sending the request body because it won't be processed, and instead it should immediately read the response to discover the 429 Too Many Requests error code.

However, Hyper's client implementation appears to just return the RST_STREAM message as an error and discards the response instead of handling it, which gives a hilariously confusing error message of:

error reading a body from connection: stream error received: not a result of an error

To be compliant with the spec, the implementation should stop sending the body and immediately read the response and return it.

For context, I'm using the Gcloud Storage API via https://crates.io/crates/aws-sdk-s3 (because the Gcloud Rust SDK doesn't support streaming bodies, but thankfully Gcloud Storage exposes an S3-compatible API), which uses Hyper internally. aws-sdk-s3 appears to be returning the error from Hyper verbatim, however.

seanmonstar commented 2 years ago

Yea, I think we've talked about this in a previous issue, but don't remember where. h2 is making the "error" (the reset) trump any other frames that have been received. It should likely be changed to return all other received frames, and then return the error.

abonander commented 2 years ago

But somewhere in the stack it should probably just suppress the RST_STREAM(NO_ERROR) and return the response, because the response is what's going to be meaningful to the user. The RST_STREAM here is just being used as a "shut up and listen" signal.

seanmonstar commented 2 years ago

Yes, it should return the response, that's why I mean. And then the body can return that there was a NO_ERROR error. It should still be given to the user, so they know something happened.

slinkydeveloper commented 1 year ago

Stumbled on this. How can I suppress the specific failure RST_STREAM(NO_ERROR) somehow? How can I workaround this? I'm also in for contributing this fix :)

DDtKey commented 1 year ago

And then the body can return that there was a NO_ERROR error. It should still be given to the user, so they know something happened.

I don’t think it should be propagated at all: The response body could be totally fine, there is no errors there, but NO_ERROR happened due to request body being dropped. It affects h2 stream status, but not the request result. So why we should to enrich the response body with the error? Only if something like “state” to check if there were underlying protocol errors, Idk

That’s actually legit scenario when body of request wasn’t consumed because server already responded back with valid response.And seems this behavior corresponds the H2 specification.

This is already long story, started here - https://github.com/hyperium/h2/pull/600#issuecomment-1645579652, and then the behavior was changed here (what is actually correct). And I just extended the test in h2 to make it more clear (https://github.com/hyperium/h2/pull/703)

UPD: I've prepared the fix on hyper client level

slinkydeveloper commented 11 months ago

Hi, any chance we could get this in a release soon?