-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Update MsQuic and use ConnectionCloseStatus in StreamShutdown #73563
Conversation
Tagging subscribers to this area: @dotnet/ncl |
Interestingly enough, the current version when building main already reports 2.1.0, and since CI does not crash, it seems to be recent enough version. |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs
Outdated
Show resolved
Hide resolved
@@ -543,12 +543,12 @@ private unsafe int HandleEventShutdownComplete(ref SHUTDOWN_COMPLETE data) | |||
(shutdownByApp: true, closedRemotely: true) => ThrowHelper.GetConnectionAbortedException((long)data.ConnectionErrorCode), | |||
// It's local shutdown by app, this side called QuicConnection.CloseAsync, throw QuicError.OperationAborted. | |||
(shutdownByApp: true, closedRemotely: false) => ThrowHelper.GetOperationAbortedException(), | |||
// It's remote shutdown by transport, (TODO: we should propagate transport error code), throw QuicError.InternalError. | |||
// TODO: we should propagate transport error code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the removal of the description // It's remote shutdown by transport,
in the comment intentional? I don't mind it since the named variables are pretty self-explanatory. But if we're doing it, could you do it for all the cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the comments back, with very brief explanation when the two transport shutdown cases occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failures are unrelated |
Fixes #73229.
This PR consumes latest MsQuic changes (2.1.0).
Opening as Draft until dependencies updates are merged (#72934). Unit tests pass with locally built MsQuic release/2.1 branch.