[release/8.0] (http2): Lower WINDOWS_UPDATE received on (half)closed stream to stream abortion #63903
Conversation
DeagleGross
requested review from
BrennanConroy,
JamesNK and
halter73
as code owners
There was a problem hiding this comment.
Pull Request Overview
Adjusts Kestrel's HTTP/2 handling so that a WINDOW_UPDATE received after a client-initiated RST_STREAM results in a stream-level error instead of a connection-level GOAWAY, reducing unnecessary connection teardown and improving client resilience.
- Change exception from connection-level (Http2ConnectionErrorException) to stream-level (Http2StreamErrorException).
- Update accompanying test to assert a stream error and perform a graceful connection stop afterward.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs | Alters handling of WINDOW_UPDATE after RST_STREAM to throw stream-level exception; adds explanatory comment. |
| src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs | Updates test to expect stream error and adjusts ordering to delay response start until after abort. |
| // stream in the "closed" state due to a reset by client. We surface it as a stream error (STREAM_CLOSED) | ||
| // rather than aborting the entire connection to keep behavior deterministic and consistent with other servers. | ||
| // https://github.com/dotnet/aspnetcore/issues/63726 | ||
| throw new Http2StreamErrorException(_incomingFrame.StreamId, CoreStrings.Http2StreamAborted, Http2ErrorCode.STREAM_CLOSED); |
There was a problem hiding this comment.
_incomingFrame.StreamId should always match stream.StreamId at this point, but using stream.StreamId would make the invariant explicit and reduce the chance of discrepancies if future changes adjust frame routing logic. Suggest replacing _incomingFrame.StreamId with stream.StreamId for clarity.
Copilot uses AI. Check for mistakes.
|
Test failure unrelated |
3 participants
Backport of #63873 to release/8.0
(http2): Lower WINDOWS_UPDATE received on (half)closed stream to stream abortion
Change affects the server HTTP/2 behavior in the case client sends the RST_STREAM frame (moving stream to half-closed or closed state) and then sends another packet - WINDOW_UPDATE. Server behavior varies in this case (RFC HTTP/2 does not describe this case extremely clear) - for example HTTP.SYS does interpet this as a stream-level error (ignore of WINDOW_UPDATE packet); but Kestrel is more strict and sends the GO_AWAY packet closing not only the stream, but also the whole connection.
Such restrictive behavior of Kestrel impacts the client, which observes cancellations and has to re-establish the connection frequently.
Fixes #63726
Customer Impact
1P Customer which processes intensive traffic of another 1P customer with a Golang-based client observes high volume of cancellations. This not only impacts the performance where a new connection has to be established, but also means all streams existing on the connection were lost, resulting in bunch of customers seeing high latency.
Regression?
Risk
Small change in behavior making Kestrel less restrictive. Also is being tested by the 1P team.
Verification
Packaging changes reviewed?
When servicing release/2.3