Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
abonander opened this issue May 26, 2022 · 6 comments · Fixed by #3275
Closed

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

abonander opened this issue May 26, 2022 · 6 comments · Fixed by #3275
Labels
A-http2 Area: HTTP/2 specific. C-bug Category: bug. Something is wrong. This is bad!

Comments

@abonander
Copy link
Contributor

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.

@abonander abonander added the C-bug Category: bug. Something is wrong. This is bad! label May 26, 2022
@seanmonstar
Copy link
Member

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.

@seanmonstar seanmonstar added the A-http2 Area: HTTP/2 specific. label May 26, 2022
@abonander
Copy link
Contributor Author

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
Copy link
Member

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
Copy link

slinkydeveloper commented Nov 4, 2022

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
Copy link
Contributor

DDtKey commented Jul 21, 2023

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 - hyperium/h2#600 (comment), and then the behavior was changed here (what is actually correct). And I just extended the test in h2 to make it more clear (hyperium/h2#703)

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

DDtKey added a commit to DDtKey/hyper that referenced this issue Jul 23, 2023
Closes hyperium#2872 for `0.14.x` version.

Added test is failing against version without these changes.
DDtKey added a commit to DDtKey/hyper that referenced this issue Jul 23, 2023
Closes hyperium#2872

Added test is failing against version without these changes.
seanmonstar pushed a commit that referenced this issue Jul 26, 2023
@slinkydeveloper
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http2 Area: HTTP/2 specific. C-bug Category: bug. Something is wrong. This is bad!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants