-
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
[QUIC] Try every ip address on host connection #99204
[QUIC] Try every ip address on host connection #99204
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsIterates every IP address that we get from DNS resolution and try to connect each of them. Contributes to #82404.
|
if (options.HandshakeTimeout != Timeout.InfiniteTimeSpan && options.HandshakeTimeout != TimeSpan.Zero) | ||
{ | ||
linkedCts.CancelAfter(options.HandshakeTimeout); | ||
} |
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.
Not sure if this is desired behavior, but currently if we are not successful to connect every ip address that we get from DNS resolution, we're going to throw this:
System.Net.Sockets.SocketException : A socket operation was attempted to an unreachable host.
instead of ConnectionTimeout
cc @ManickaP
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.
IMO unreachable host is better error message than connection timeout. ConnectionTimeout implies we reached the host but the host was slow in completing the connection.
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 think in sockets the exception depends on the condition. If the port is closed and actively refusing connection via RST or ICMP we would get unreachable error, if it fails on timeout e.g. no response we would throw the timeout. Note that for tests on loopback there is observable difference between Windows and Linux.
Tagging subscribers to this area: @dotnet/ncl Issue DetailsIterates every IP address that we get from DNS resolution and try to connect each of them. Contributes to #82404.
|
src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
Outdated
Show resolved
Hide resolved
{ | ||
if (options.HandshakeTimeout != Timeout.InfiniteTimeSpan && options.HandshakeTimeout != TimeSpan.Zero) | ||
{ | ||
linkedCts.CancelAfter(options.HandshakeTimeout); |
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.
This feels this is wrong. I think the options.HandshakeTimeout
should cover timeout for ConnectAsync
itself not for each address we try. In practice the observed timeout would depend on number of addresses given host resolves to.
But we do not have any real explanation
https://learn.microsoft.com/en-us/dotnet/api/system.net.quic.quicconnectionoptions.handshaketimeout?view=net-9.0
On the other hand, It already has CancellationToken
to it somewhat feels duplicate.
Iterates every IP address that we get from DNS resolution and try to connect each of them.
Contributes to #82404.