Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented May 6, 2021

Description

An HTTP/2 client sends multiple RST_STREAM frames for the same stream to Kestrel (this is a spec violation by the client, but the client's violation is exposing a violation in Kestrel)

If both RST_STREAM frames are read from the transport at the same time, Kestrel is incorrectly killing the collection. Kestrel is violating the HTTP/2 specification.

The correct logic is to ignore the second RST_STREAM frame. PR corrects that logic.

This PR takes the changes in #32449 and backports them to 5.0

Customer Impact

Customer using gRPC Java client to call Kestrel. Kestrel is ending the connection when this bug occurs, causing the customer app to break.

#32449 (comment)

It is unlikely that this bug impacts many people. An HTTP/2 client that violates the spec and the right situation is required to encounter it.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

Low risk. The connection error on receiving the second RST_STREAM frame is replaced with an ignore. This makes Kestrel match the HTTP/2 spec.

Verification

  • Manual (required)
  • Automated

Manually verifying this would be tough. It would require setting up Java client and replicating its code. Might need to ask the customer to retest with 6.0 daily build that contains the fix.

Update: Customer has verified fix using .NET 6 - #32478 (comment)

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Addresses #32442

@JamesNK JamesNK added Servicing-consider Shiproom approval is required for the issue area-runtime labels May 6, 2021
@JamesNK JamesNK added this to the 5.0.x milestone May 6, 2021
@ghost
Copy link

ghost commented May 6, 2021

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@ahjohannessen
Copy link

Thanks for doing this @JamesNK - I tried to test this patch locally with EventStoreDB and the problem is fixed wrt. not getting GOAWAY! :)

@leecow leecow modified the milestones: 5.0.x, 5.0.8 May 11, 2021
@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels May 11, 2021
@wtgodbe wtgodbe merged commit bfc145d into release/5.0 Jun 1, 2021
@wtgodbe wtgodbe deleted the jamesnk/http2-multiplerststream-50 branch June 1, 2021 21:57
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants