Fix sending end stream in http2 web socket stream #73222
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #72301
|
src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.Http2.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
Outdated
Show resolved
Hide resolved
greenEkatherine
force-pushed
the
fix-end-stream-ws-h2
branch
from
2fedcf6
to
c88e46b
Compare
src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.Http2.cs
Outdated
Show resolved
Hide resolved
…st.Http2.cs Co-authored-by: Natalia Kondratyeva <[email protected]>
|
@stephentoub please can you code review? Thanks! |
src/libraries/System.Net.WebSockets.Client/tests/ConnectTest.Http2.cs
Outdated
Show resolved
Hide resolved
…ttp2.cs Co-authored-by: Stephen Toub <[email protected]>
greenEkatherine
force-pushed
the
fix-end-stream-ws-h2
branch
from
95d9cfd
to
d2d3ff8
Compare
| @@ -1599,7 +1599,7 @@ private async ValueTask<Http2Stream> SendHeadersAsync(HttpRequestMessage request | |||
| // Start the write. This serializes access to write to the connection, and ensures that HEADERS | |||
| // and CONTINUATION frames stay together, as they must do. We use the lock as well to ensure new | |||
| // streams are created and started in order. | |||
| await PerformWriteAsync(totalSize, (thisRef: this, http2Stream, headerBytes, endStream: (request.Content == null), mustFlush), static (s, writeBuffer) => | |||
| await PerformWriteAsync(totalSize, (thisRef: this, http2Stream, headerBytes, endStream: (request.Content == null && !http2Stream.ConnectProtocolEstablished), mustFlush), static (s, writeBuffer) => | |||
There was a problem hiding this comment.
Maybe I'm doing something wrong, or misunderstanding the change, but if I'm reading this correctly and testing the new feature correctly, this check does nothing because ConnectProtocolEstablished is only set after the request has already been sent and a response with a 200 was received.
Which means every time an HTTP2 WebSocket request is made it will still send the END_STREAM flag.
Did you test this change with Kestrel before merging? Am I holding the feature wrong?
cc @karelz
There was a problem hiding this comment.
Sorry, I will double check. I tested it with Kestrel but I probably miss to check state on the server side. Using receive before send works so I thought that the end stream flag causes an issue only on established connection. Also connect without end stream is hanging, I should try different approach.
There was a problem hiding this comment.
I see, I have to switch on flush correctly for both connect and send
ghost
locked as resolved and limited conversation to collaborators
Fixes #72301