Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[H/3] Fix code passed into QuicConnection.CloseAsync and QuicStream.Abort #55282

Merged
merged 11 commits into from
May 3, 2024

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Apr 22, 2024

Fixes the code passed into QuicConnection.CloseAsync and QuicStream.Abort which was by default -1. Quic supports only error codes up to 62 bits non-negative integer values: https://www.rfc-editor.org/rfc/rfc9000.html#integer-encoding.
Also added test which crashes the process (if running with DEBUG build of MsQuic) before this change and runs to completion afterwards.

Let me know if you want me to shuffle the code around, I'm not much familiar with the structure of your code base.

cc @amcasey

Fixes #55196

Also this will be followed up in S.N.Quic with some checks (dotnet/runtime#101385).

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 22, 2024
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to flesh out the test a little, but I'm okay merging as-is if this unblocks you.

@amcasey amcasey enabled auto-merge (squash) April 24, 2024 19:29
@amcasey
Copy link
Member

amcasey commented Apr 24, 2024

My new check is failing CI. The test fails for me locally, but not until after the log message check succeeds, so I'm not sure why that's happening. I'll investigate.

Edit: it was flaky before my change.

@amcasey
Copy link
Member

amcasey commented Apr 25, 2024

The windows failure is known CI flakiness

@amcasey
Copy link
Member

amcasey commented Apr 25, 2024

Okay, looking more closely, the failure I'm seeing here is the same one I'm seeing locally and isn't related to the logging check. I think the test is flaky in that it won't reliably cause the connection to be closed on the server side.

@amcasey
Copy link
Member

amcasey commented May 2, 2024

I believe the PR is ready to go.

@amcasey amcasey merged commit b37eafd into dotnet:main May 3, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview5 milestone May 3, 2024
@amcasey
Copy link
Member

amcasey commented May 3, 2024

Ah, it was auto-merge from the previous design and my sign-off counts because it's technically Mana's PR. Well, shout if you don't like the exceptions @halter73.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QUIC] QuicConnectionContext.Abort calls connection shutdown with error code -1
3 participants