Handle multiple RST_STREAM frames

[jonathanjameswatson]

When a client receives two RST_STREAM frames, a NYI exception is raised. This is not ideal, as servers are allowed to send multiple RST_STREAM frames.

From RFC7540§5.4.2:

A RST_STREAM is the last frame that an endpoint can send on a stream. The peer that sends the RST_STREAM frame MUST be prepared to receive any frames that were sent or enqueued for sending by the remote peer. These frames can be ignored, except where they modify connection state (such as the state maintained for header compression Section 4.3 or flow control).

Normally, an endpoint SHOULD NOT send more than one RST_STREAM frame for any stream. However, an endpoint MAY send additional RST_STREAM frames if it receives frames on a closed stream after more than a round-trip time. This behavior is permitted to deal with misbehaving implementations.

The first RST_STREAM frame should close the stream. In the closed state, extra RST_STREAM frames should be ignored (unless they arrive after a "significant" amount of time).

From RFC7540§5.1:

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.

Here is a relevant issue:

dotnet/aspnetcore#32442

I have added a test for this problem, and will add a case to solve it soon.

[anmonteiro]

Thanks for this, I’ll look over it soon. Note that we might need to do the same for the server implementation (up to you whether you add it in this PR or another one)

[jonathanjameswatson]

Looking at the match statements in server_connection.ml's process_rst_stream_frame, I would expect the server to not run into this problem. I'll try to write a test to check.

[jonathanjameswatson]

I have fixed the client and added a server test (that might need improvement), which passes.

[anmonteiro]

I added a new test verifying we also behave correctly for 2 RST_STREAM frames with a No_error payload.

I have fixed the client and added a server test (that might need improvement), which passes.

Looking over the code in server_connection, I feel like what might be happening is that we aren't ever calling the stream-level error handler. That's why your test passes.

A follow-up PR could unify the behavior with the client implementation, if it makes sense.

[jonathanjameswatson]

Sounds good! That would also be useful for solving an issue I'll likely raise tomorrow where it seems that the server continues to send RESET_STREAM and DATA frames after sending a RESET_STREAM frame due to a malformed content-length header.

[jonathanjameswatson]

I added a new test verifying we also behave correctly for 2 RST_STREAM frames with a No_error payload.

Also, this makes me think that the error code might not be relevant, as far as the above parts of the protocol mention. Would it make sense to have the match statement in client_connection look like the following instead:

(match respd.state, error_code with
| Idle, _ -> ...
| Closed _, _ -> ()
| Active _, Error_code.NoError -> ...
| _ -> ...)

[anmonteiro]

I applied your suggestion, thanks!