-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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] Throw meaningful exception if we get GOAWAY while reading response body #104707
Conversation
…ng for next frame on response
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see this was a simple enough change to get into 9.0.
Thanks
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Outdated
Show resolved
Hide resolved
Is the original issue an issue at all? Based on JNK logs:
He complains that server didn't let the client finish stream Id 1, while sending GO_AWAY with id 1. What does the RFC says? I'd expect server is supposed to let us finish those requests. |
RFC9113 - HTTP/2 says:
If I'm interpreting this correctly, this can go either way. |
I think @JamesNK should be able to test private binaries to confirm. |
That would be nice, but the test is written as he described in the reproduction steps. So, I think this kind of confirms that it's behaving as expected. |
What do you mean by "let us finish those requests"? Are you expecting reading from the response stream so say there is no more data without throwing an error? Because of the GO_AWAY the server is never going to send a final stream frame with END_STREAM on it. The stream ended ungracefully. I expect an error when trying to read more of the stream body. What I want improved is the exception's message to say why the response ended. |
Something that occurred to me, can responseStream.ReadAsync throw |
That's a great question, so you're saying that this can be breaking change. I'll leave the answer to the HTTP experts. |
Is it breaking change? Maybe. I don't know the semantics of when to throw an |
|
Fixes #97128
Not sure if this is the correct way to fix this.