HTTP/2: Correctly handle receiving multiple RST_STREAM frames

[JamesNK]

grpc/grpc-java#8142

tldr; Java gRPC client is sending two RST_STREAM frames for the same stream ID. Kestrel is killing the connection. This might not be correct.

Stream states section in the HTTP/2 spec: https://tools.ietf.org/html/rfc7540#section-5.1

When Kestrel receives the second RST_STREAM frame, it goes into a closed state.

  An endpoint MUST NOT send frames other than PRIORITY on a closed
  stream.  An endpoint that receives any frame other than PRIORITY
  after receiving a RST_STREAM MUST treat that as a stream error
  (Section 5.4.2) of type STREAM_CLOSED.  Similarly, an endpoint
  that receives any frames after receiving a frame with the
  END_STREAM flag set MUST treat that as a connection error
  (Section 5.4.1) of type STREAM_CLOSED, unless the frame is
  permitted as described below.

Kestrel should have a stream error rather than a connection error on the second RST_STREAM.

I'm not sure what a stream error would mean in this case as you're not allowed to send a RST_STREAM in response to a RST_STREAM - https://tools.ietf.org/html/rfc7540#section-5.4.2

An endpoint that detects a stream error sends a RST_STREAM frame
(Section 6.4) that contains the stream identifier of the stream where
the error occurred. The RST_STREAM frame includes an error code that
indicates the type of error.

And

To avoid looping, an endpoint MUST NOT send a RST_STREAM in response
to a RST_STREAM frame.

I think Kestrel should just ignore additional RST_STREAM frames.

[JamesNK]

@halter73 @Tratcher

[ejona86]

This might be the better reference from that same section:

 Endpoints MUST ignore
 WINDOW_UPDATE or RST_STREAM frames received in this state, though
 endpoints MAY choose to treat frames that arrive a significant
 time after sending END_STREAM as a connection error
 (Section 5.4.1) of type PROTOCOL_ERROR.

The problem is related to this in §6.1 that says to send RST_STREAM:

If a DATA frame is received
whose stream is not in "open" or "half-closed (local)" state, the
recipient MUST respond with a stream error (Section 5.4.2) of type
STREAM_CLOSED.

The Netty code used to keep state associated with closed streams to avoid double-sending RST_STREAMs, but that took too much memory so after lots of back-and-forth of adding/removing/re-adding-but-smaller/re-removing the state, the current behavior was arrived at.

[JamesNK]

I think that bit of text is covering the situation where the server gets into a closed state by the client and server both sending frames with END_STREAM flags.

In this situation, we get into the closed state by the server receiving RST_STREAM.

[ejona86]

The language starts with END_STREAM, and pulls in RST_STREAM as well. More context:

WINDOW_UPDATE or RST_STREAM frames can be received in this state
for a short period after a DATA or HEADERS frame containing an
END_STREAM flag is sent. Until the remote peer receives and
processes RST_STREAM or the frame bearing the END_STREAM flag, it
might send frames of these types. Endpoints MUST ignore...

How does Kestrel avoid sending multiple RST_STREAMS?

[Tratcher]

I think Kestrel should just ignore additional RST_STREAM frames.

Agreed.

[JamesNK]

How does Kestrel avoid sending multiple RST_STREAMS?

Kestrel keeps track of completed streams for a short amount of time. This isn't just to avoid sending multiple RST_STREAM frames. They are used to handle situations like the server aborting a stream but frames for that stream are still arriving from the client. Frames arriving for a stream ID that no longer exists is a protocol error so we have a grace period to ignore them.

[halter73]

Kestrel keeps track of completed streams for a short amount of time.

IIRC, this is only for the streams that kestrel itself RSTs because the client might not be aware of it. If the client sends the RST, Kestrel stops tracking it immediately because the client should be completely done with them. Because of this, Kestrel cannot distinguish between a stream that a client reset and a stream that never existed.

To ignore the additional RST_STREAM frames, we'd have to either:

1. Continue tracking streams after receiving a RST_STREAM frames, but it feels bad to me to hold on to extra memory longer just to allow for better interaction with clients committing protocol violations.
2. Ignore all frames to unknown stream IDs, but this could make it harder to uncover other possibly more common protocol violations.
3. Continue treating this as a connection error like we do today.

[halter73]

The other option would be to treat frames to unknown stream IDs as a stream error, but that's a non-starter because if you have two HTTP/2 implementations that do that, you could end up in an infinite RST_STREAM loop.

[JamesNK]

The error message in this situation still has the stream tracked by Kestrel when the second RST_STREAM arrives.

| [    1,28,09:32:24.896,VRB] Connection id ""0HM8FKDKJOM5B"" received RST_STREAM frame for stream ID 1063 with length 4 and flags "0x0"
| [    1,28,09:32:24.896,VRB] Connection id ""0HM8FKDKJOM5B"" received RST_STREAM frame for stream ID 1063 with length 4 and flags "0x0"
| [    1,28,09:32:24.897,DBG] Connection id ""0HM8FKDKJOM5B"": HTTP/2 connection error.
| Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2ConnectionErrorException: HTTP/2 connection error (STREAM_CLOSED): A frame of type RST_STREAM was received after stream 1063 was reset or aborted.
|    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2Connection.ProcessRstStreamFrameAsync()
|    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2Connection.ProcessFrameAsync[TContext](IHttpApplication`1 application, ReadOnlySequence`1& payload)
|    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2Connection.ProcessRequestsAsync[TContext](IHttpApplication`1 application)

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

Lines 715 to 726 in c671359

	if (_streams.TryGetValue(_incomingFrame.StreamId, out var stream))
	{
	// Second reset
	if (stream.RstStreamReceived)
	{
	// Hard abort, do not allow any more frames on this stream.
	throw new Http2ConnectionErrorException(CoreStrings.FormatHttp2ErrorStreamAborted(_incomingFrame.Type, stream.StreamId), Http2ErrorCode.STREAM_CLOSED);
	}
	// No additional inbound header or data frames are allowed for this stream after receiving a reset.
	stream.AbortRstStreamReceived();
	}

[JamesNK]

Ah, I think it is only still tracked because Kestrel received both RST_STREAM frames in the same read from the transport. Once Kestrel gets more data from the transport it will run UpdateCompletedStreams() and remove completed streams from the dictionary.

However, it looks like ProcessRstStreamAsync will no-op if the streamId isn't in the streams dictionary.

[halter73]

Nice! I didn't realize we were ignoring RST_STREAM frames to unknown streams already. Making it work the same if the stream is still in the dictionary but already reset sounds good to me.