ignore invalid frames on closed streams#38918
Conversation
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Show resolved
Hide resolved
|
I pushed another change to also ignore frame processing callbacks on aborted streams. This seems to make all the stress issues here go away. Please take a look. |
|
I pushed one additional change: The connection-level window accounting on cancelled streams was incorrect. It would not account for frames received after cancellation, meaning that over time the window would effectively lose credit and shrink, eventually to 0. The fix is to just directly account for the connection-level window in ProcessDataFrame. This has the additional benefit of being simpler and more obviously correct. |
| await WriteFrameAsync(ping).ConfigureAwait(false); | ||
| await ReadFrameAsync(Timeout).ConfigureAwait(false); | ||
| Frame pingAck = await ReadFrameAsync(Timeout).ConfigureAwait(false); | ||
| if (pingAck.Type != FrameType.Ping || !pingAck.AckFlag) |
There was a problem hiding this comment.
nit: something we might want to add is also checking if bytes match
| Frame pingAck = await ReadFrameAsync(Timeout).ConfigureAwait(false); | ||
| if (pingAck.Type != FrameType.Ping || !pingAck.AckFlag) | ||
| { | ||
| throw new Exception("Expected PING ACK"); |
There was a problem hiding this comment.
Why is it throwing Exception? Usually these are Http2ProtocolException with a suitable error number etc.
…closedstream ignore invalid frames on closed streams Commit migrated from dotnet/corefx@b9c89c4
Contributes to #38913
Contributes to #35652
Ignore any frames received on a closed stream -- more specifically, on a stream that is no longer in the stream dictionary.
We don't distinguish between streams closed because they were reset by the client (due to cancellation etc) or streams closed because they completed. The former is valid, while the latter is technically invalid per RFC. However, to avoid having to track why the stream was closed, we just ignore these frames in both cases. This matches WinHttp behavior.
We are still not handling the case where a stream is cancelled during the processing of a HEADER or DATA frame.
@dotnet/ncl