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.

Sign up for GitHub

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

Jump to bottom

[QUIC] Try every ip address on host connection #99204

Closed
wants to merge 4 commits into from
Closed

[QUIC] Try every ip address on host connection #99204

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78ff704
QUIC traverse all ip addresses and connect on host connection
liveans Mar 3, 2024
5ea605f
Check if connection disposed, before creating new one
liveans Mar 4, 2024
e819731
Reuse configuration
liveans Mar 5, 2024
fdbf47a
Merge branch 'main' into quic_connect_multiple_ip_address_from_host
liveans Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 54 additions & 31 deletions src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,64 @@ public static ValueTask<QuicConnection> ConnectAsync(QuicClientConnectionOptions

static async ValueTask<QuicConnection> StartConnectAsync(QuicClientConnectionOptions options, CancellationToken cancellationToken)
{
MsQuicSafeHandle? config = MsQuicConfiguration.Create(options);
QuicConnection connection = new QuicConnection();
connection._configuration = config;

using CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

if (options.HandshakeTimeout != Timeout.InfiniteTimeSpan && options.HandshakeTimeout != TimeSpan.Zero)
{
linkedCts.CancelAfter(options.HandshakeTimeout);
}

try
{
await connection.FinishConnectAsync(options, linkedCts.Token).ConfigureAwait(false);
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));
}

if (address is not null && host is null)
{
if (options.HandshakeTimeout != Timeout.InfiniteTimeSpan && options.HandshakeTimeout != TimeSpan.Zero)
{
linkedCts.CancelAfter(options.HandshakeTimeout);
Copy link
Member

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.

}
Comment on lines +83 to +86
Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.


await connection.FinishConnectAsync(options, null, address, port, linkedCts.Token).ConfigureAwait(false);
return connection;
}

Debug.Assert(host is not null);
IPAddress[] addresses = await Dns.GetHostAddressesAsync(host, cancellationToken).ConfigureAwait(false);
cancellationToken.ThrowIfCancellationRequested();
if (addresses.Length == 0)
{
throw new SocketException((int)SocketError.HostNotFound);
}

ExceptionDispatchInfo? lastException = null;

foreach (IPAddress addr in addresses)
{
if (connection._disposed != 0)
{
connection = new QuicConnection();
connection._configuration = config;
}
try
{
await connection.FinishConnectAsync(options, host, addr, port, linkedCts.Token).ConfigureAwait(false);
lastException = null;
config = null;
break;
}
catch (Exception ex)
{
lastException = ExceptionDispatchInfo.Capture(ex);
connection._configuration = null; // Ignore disposal of config on failure, to reuse it.
await connection.DisposeAsync().ConfigureAwait(false);
}
}

config?.Dispose();
lastException?.Throw();
}
catch (OperationCanceledException)
{
Expand Down Expand Up @@ -289,36 +335,14 @@ internal unsafe QuicConnection(QUIC_HANDLE* handle, QUIC_NEW_CONNECTION_INFO* in
#endif
}

private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, CancellationToken cancellationToken = default)
private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, string? host, IPAddress address, int port, CancellationToken cancellationToken = default)
{
if (_connectedTcs.TryInitialize(out ValueTask valueTask, this, cancellationToken))
{
_canAccept = options.MaxInboundBidirectionalStreams > 0 || options.MaxInboundUnidirectionalStreams > 0;
_defaultStreamErrorCode = options.DefaultStreamErrorCode;
_defaultCloseErrorCode = options.DefaultCloseErrorCode;

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));
}

if (address is null)
{
Debug.Assert(host is not null);

// Given just a ServerName to connect to, msquic would also use the first address after the resolution
// (https://github.com/microsoft/msquic/issues/1181) and it would not return a well-known error code
// for resolution failures we could rely on. By doing the resolution in managed code, we can guarantee
// that a SocketException will surface to the user if the name resolution fails.
IPAddress[] addresses = await Dns.GetHostAddressesAsync(host, cancellationToken).ConfigureAwait(false);
cancellationToken.ThrowIfCancellationRequested();
if (addresses.Length == 0)
{
throw new SocketException((int)SocketError.HostNotFound);
}
address = addresses[0];
}

QuicAddr remoteQuicAddress = new IPEndPoint(address, port).ToQuicAddr();
MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_REMOTE_ADDRESS, remoteQuicAddress);

Expand All @@ -336,7 +360,6 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options,
options.ClientAuthenticationOptions.CertificateRevocationCheckMode,
options.ClientAuthenticationOptions.RemoteCertificateValidationCallback,
options.ClientAuthenticationOptions.CertificateChainPolicy?.Clone());
_configuration = MsQuicConfiguration.Create(options);

// RFC 6066 forbids IP literals.
// IDN mapping is handled by MsQuic.
Expand All @@ -349,7 +372,7 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options,
{
ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ConnectionStart(
_handle,
_configuration,
_configuration!,
(ushort)remoteQuicAddress.Family,
(sbyte*)targetHostPtr,
(ushort)port),
Expand Down
Loading