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

HTTP/2 should send a reset of NO_ERROR instead of CANCEL if already sent response headers #2729

Closed
maxbear1988 opened this issue Dec 30, 2021 · 13 comments
Labels
A-http2 Area: HTTP/2 specific. C-bug Category: bug. Something is wrong. This is bad! E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.

Comments

@maxbear1988
Copy link

maxbear1988 commented Dec 30, 2021

I write a proxy service that use hyper.I upload a file by chrome browser use http2 to my proxy service.In my proxy service,i check the file size by request header content-length and find it is too large, i retrun response with status code 413 without reading the request body immediately.chrome report ERR_HTTP2_PROTOCOL_ERROR, but it is ok in http1.1.
I read http2.0 protocol rfc7540 and find program should send RST_STREAM frame with an error code of NO ERROR after sending a complete response.
Does hyper provide a solution to this problem?How to solve the problem?

@maxbear1988 maxbear1988 added the C-bug Category: bug. Something is wrong. This is bad! label Dec 30, 2021
@seanmonstar
Copy link
Member

hyper does send a reset frame after you've responded and dropped the request body without reading.

The error seems to suggest it's related to SSL, not to HTTP/2.

@maxbear1988
Copy link
Author

@seanmonstar thank you. i don't think it is related to SSL. It it performance well in http1.1;if i read the request body,it performance well in http2.0,too. Does hyper send a reset frame with an error code of NO ERROR already?in which version?i don‘t see this code by searching keyword send_reset?I only find send_reset(h2::Reason::INTERNAL_ERROR)

@seanmonstar
Copy link
Member

The automatic sending of a reset is handled in h2, a dependency of hyper.

@maxbear1988 maxbear1988 changed the title chrome browser fail to access the service that use hyper by http2.0 and report ERR_SSL_PROTOCOL_ERROR chrome browser fail to access the service that use hyper by http2.0 and report ERR_HTTP2_PROTOCOL_ERROR Jan 18, 2022
@maxbear1988
Copy link
Author

@seanmonstar chrome report ERR_HTTP2_PROTOCOL_ERROR ,not ERR_SSL_PROTOCOL_ERROR ,i wrote error last time

@seanmonstar
Copy link
Member

Does this happen with other clients? Firefox? curl? Some other language client?

@maxbear1988
Copy link
Author

@seanmonstar yes,i try chrome,firefox,Microsoft Edge,all report ERR_HTTP2_PROTOCOL_ERROR

@maxbear1988
Copy link
Author

@seanmonstar if i read all request body, then response status code 413, it is ok. if i response status code 413 without reading request body, it is not ok.I think hyper should send RST_STREAM frame with error code of NO ERROR after respone status code 413 without waiting for all request body.

@maxbear1988
Copy link
Author

@seanmonstar i open hyper trace log,hyper send reset frame with error_code CANCEL. FramedWrite::buffer�[0m�[1m{�[0mframe=Reset { stream_id: StreamId(647), error_code: CANCEL

@seanmonstar seanmonstar added A-http2 Area: HTTP/2 specific. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. labels Jan 21, 2022
@seanmonstar
Copy link
Member

Ah, I see. So currently, if you drop all handles to the stream while there is still things to read, it will automatically send a reset of CANCEL. The h2 library should be updated to check if (a) we are the server, and (b) if response headers have already been sent, then change to sending a reset of NO_ERROR instead.

@maxbear1988
Copy link
Author

@seanmonstar do you have a plan to fix this issue?

@seanmonstar seanmonstar changed the title chrome browser fail to access the service that use hyper by http2.0 and report ERR_HTTP2_PROTOCOL_ERROR HTTP/2 should send a reset of NO_ERROR instea of CANCEL if already sent response headers Feb 8, 2022
@seanmonstar seanmonstar changed the title HTTP/2 should send a reset of NO_ERROR instea of CANCEL if already sent response headers HTTP/2 should send a reset of NO_ERROR instead of CANCEL if already sent response headers Feb 8, 2022
@seanmonstar
Copy link
Member

I have not prioritized fixing this myself, but we welcome contributions if you'd like to join in, or anyone else :)

@arnauorriols
Copy link

I believe this issue was fixed in hyperium/h2#634

@seanmonstar
Copy link
Member

Ah yes!

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! E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Projects
None yet
Development

No branches or pull requests

3 participants