-
Notifications
You must be signed in to change notification settings - Fork 5.2k
throw SocketException for Quic transport errors #87208
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
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue Detailscontributes to #82262 note that on Linux there are two different modes of IPv6 not available. That mode is generally not applicable to Windows see microsoft/msquic#3093 (comment) One can also set
|
| throw new ArgumentNullException(SR.Format(SR.net_quic_not_null_open_connection, nameof(QuicClientConnectionOptions.RemoteEndPoint)), argumentName); | ||
| } | ||
|
|
||
| // MsQuic is using DualMode sockets and that will faill even for IPv4 if AF_INET6 is not available. |
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 don't think this should be part of options Validation. Can you move this to connect itself? It also should not be thrown synchronously from ConnectAsync, but rather exposed as stored exception in the task.
General guidance is that only Argument validation exceptions should be thrown synchronously from async methods. What socket does is not 100% in line with this guidance, but we keep these inconsistencies for historical reasons.
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 can move it but I felt it would be better to keep everything together. And I also feel that throwing synchronously is better for cases when we even cannot start the operation. Is the guidance written somewhere?
cc: @stephentoub for more insight.
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.
If quic can't work without OSSupportsIPv6, shouldn't that instead be part of QuicConnection.IsSupported, e.g.
public static bool IsSupported => MsQuicApi.IsQuicSupported && Socket.OSSupportsIPv6;And for RemoteEndPoint, shouldn't that validation be done as part of setting the RemoteEndPoint, e.g. move the validation of its AddressFamily into the property setter?
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 can update IsSupported. I realized that only after writing tests and testing various setups. The only caveat is that when 'IsSupportedisfalse` it is very difficult IMHO to figure out why. But I guess we can document it in remarks.
As far as validation in setter: AFAIK we don't do that or anything else - neither in Quic nor SslStream e.g. for property bags we validate when applied to some action now when they are created. We could go down that route but then I would expect same for other properties.
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.
Actually, I think moving Socket.OSSupportsIPv6 to IsSupported makes the most sense. AFAICT, it cannot change during app run and is essential for MsQuic to work.
But I'd suggest to make it part of MsQuicApi.IsQuicSupported as this might be MsQuic specific behavior and it will also give us opportunity to log this as we do with missing libmsquic etc. This should also answer the problem of diagnosing this as it would be evident from the logs what's going on. WDYT?
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.
make sense. How about the IPv4 check? It seems like less likely but sill possible. And than the question if it is ok to throw synchronously, store exception or do it when somebody is manipulating the option object.
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 would not put it into the setter, we intentionally postpone all validations until the options are used.
We do other IP related and other checks afterwards, e.g.:
runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Lines 243 to 247 in 7c00b17
| if (!options.RemoteEndPoint.TryParse(out string? host, out IPAddress? address, out int port)) | |
| { | |
| throw new ArgumentException(SR.Format(SR.net_quic_unsupported_endpoint_type, options.RemoteEndPoint.GetType()), nameof(options)); | |
| } | |
| int addressFamily = QUIC_ADDRESS_FAMILY_UNSPEC; |
runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Lines 263 to 266 in 7c00b17
| if (addresses.Length == 0) | |
| { | |
| throw new SocketException((int)SocketError.HostNotFound); | |
| } |
I still personally think this fits there better.
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 moved the IPv6 check to IsSupported as suggested. As fas as the other check I'm still not convinced. The link you posted spawns task to figure out if something is going to work. But for the address family we can tell upfront it is going to fail (without any additional work)
From my prospective if something is going to fail the sooner we surface error and we stop doing extra work the better. Throwing in validation avoids creation of the task and and executing more code. But I can move it if that is consensus.
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 left IPv6 check as part of the IsSupported and I took out IPv4 check. I'll make plan and I'll put it up together with fixes for #74053.
contributes to #82262
note that on Linux there are two different modes of IPv6 not available.
One can pass
ipv6.disable=1to boot loader. With that,Socket.OSSupportsIPv6isfalseand Quic fails to function even for IPv4 as MsQuic fails to create socket.That mode is generally not applicable to Windows see microsoft/msquic#3093 (comment)
One can also set
sysctl -w net.ipv6.conf.all.disable_ipv6=1on Linux orHKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Tcpip6\Parameters\DisabledComponents=0xffffffffon Windows. That efectively disables IPv6 on all interfaces,Socket.OSSupportsIPv6is true, IPv4 works but attempt to bind on IPv6 ANY or connect to IPv6 destination fails with error described in #74053