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

[PoC] surface Quic transport errors #87679

Closed
wants to merge 3 commits into from
Closed

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jun 16, 2023

This implements proposal from #72666.

We would currently surface TransportError on cipher mismatch. I suspect that both MsQuic and other implementations may have more codes that we do not handle so the actual set may be bigger.

Looking at the cases again when we throw on transport error, I would like to prose change on QUIC_STATUS_USER_CANCELED.

While that maps to MsQuic status, I feel cancellation has specific meaning in .NET around cancellation tokens.
In tests where this happens there is no cancellation nor user. Typically this is failure to complete handshake operation. (like callback failure)

-  System.Security.Authentication.AuthenticationException: Authentication failed because the remote party sent a TLS alert: 'UserCanceled'.
+  System.Net.Quic.QuicException: QUIC encountered transport error '346'

I also feel that when we would surface as transport error, we would be more following RFC than MsQuic internals. In either case there is not much the code can do e.g. this is purely diagnostic.

@wfurt wfurt added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-System.Net.Quic labels Jun 16, 2023
@wfurt wfurt added this to the 8.0.0 milestone Jun 16, 2023
@wfurt wfurt self-assigned this Jun 16, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 16, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This implements proposal from #72666.

We would currently surface TransportError on cipher mismatch. I suspect that both MsQuic and other implementations may have more codes that we do not handle so the actual set may be bigger.

Looking at the cases again when we throw on transport error, I would like to prose change on QUIC_STATUS_USER_CANCELED.

While that maps to MsQuic status, I feel cancellation has specific meaning in .NET around cancellation tokens.
In tests where this happens there is no cancellation nor user. Typically this is failure to complete handshake operation. (like callback failure)

-  System.Security.Authentication.AuthenticationException: Authentication failed because the remote party sent a TLS alert: 'UserCanceled'.
+  System.Net.Quic.QuicException: QUIC encountered transport error '346'

I also feel that when we would surface as transport error, we would be more following RFC than MsQuic internals. In either case there is not much the code can do e.g. this is purely diagnostic.

Author: wfurt
Assignees: wfurt
Labels:

NO-MERGE, area-System.Net.Quic

Milestone: 8.0.0

@ManickaP
Copy link
Member

I'm not a fan of this particular proposition:

-  System.Security.Authentication.AuthenticationException: Authentication failed because the remote party sent a TLS alert: 'UserCanceled'.
+  System.Net.Quic.QuicException: QUIC encountered transport error '346'

I don't think making exception more obscure is better. I don't find the combination of TLS alert and cancel misleading, but if you find it unsuitable, is there anything better we could replace this with?

@ManickaP
Copy link
Member

This part of QuicStream code is also relevant in this change:

// It's remote shutdown by transport, we received a CONNECTION_CLOSE frame with a QUIC transport error code, throw error based on the status.
// TODO: we should propagate the transport error code
// https://github.com/dotnet/runtime/issues/72666
(shutdownByApp: false, closedRemotely: true) => ThrowHelper.GetExceptionForMsQuicStatus(data.ConnectionCloseStatus, $"Shutdown by transport {data.ConnectionErrorCode}"),
// It's local shutdown by transport, most likely due to a timeout, throw error based on the status.
// TODO: we should propagate the transport error code
// https://github.com/dotnet/runtime/issues/72666
(shutdownByApp: false, closedRemotely: false) => ThrowHelper.GetExceptionForMsQuicStatus(data.ConnectionCloseStatus),

@wfurt
Copy link
Member Author

wfurt commented Jul 9, 2023

closing in favor of #88550

@wfurt wfurt closed this Jul 9, 2023
@wfurt wfurt deleted the transport branch July 9, 2023 20:44
@ghost ghost locked as resolved and limited conversation to collaborators Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Quic new-api-needs-documentation NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants