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

improve resiliency of quic test #57020

Merged
merged 3 commits into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/libraries/System.Net.Quic/ref/System.Net.Quic.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public partial class QuicException : System.Exception
{
public QuicException(string? message) { }
public QuicException(string? message, System.Exception? innerException) { }
public QuicException(string? message, System.Exception? innerException, int result) { }
}
public static partial class QuicImplementationProviders
{
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Quic/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@
<data name="net_quic_cert_chain_validation" xml:space="preserve">
<value>The remote certificate is invalid because of errors in the certificate chain: {0}</value>
</data>
<data name="net_quic_not_connected" xml:space="preserve">
<value>Connection is not connected.</value>
</data>
<data name="net_ssl_app_protocols_invalid" xml:space="preserve">
<value>The application protocol list is invalid.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Net.Sockets;

namespace System.Net.Quic.Implementations.MsQuic.Internal
{
internal static class QuicExceptionHelpers
Expand All @@ -15,7 +17,22 @@ internal static void ThrowIfFailed(uint status, string? message = null, Exceptio

internal static Exception CreateExceptionForHResult(uint status, string? message = null, Exception? innerException = null)
{
return new QuicException($"{message} Error Code: {MsQuicStatusCodes.GetError(status)}", innerException);
return new QuicException($"{message} Error Code: {MsQuicStatusCodes.GetError(status)}", innerException, MapMsQuicStatusToHResult(status));
}

internal static int MapMsQuicStatusToHResult(uint status)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is a temporary solution and we should define our own status codes that'll abstract the msquic ones.
That could be probably covered with #32066.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may change the exception base and define quic specific codes. #32066 is primarily talking about the messages.

However, the HResult I pick are generally portable and for example you can use https://www.hresult.info to look them up. What comes out should pretty match what is happening at MsQuic.
Hopefully this will not be thrown directly in 6.0 and we can finalize this when we do exception cleanup and when we make it public.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so basically we would rather expand the usage of HResult to cover all the relevant codes from msquic.
I assume this will be thrown directly from S.N.Quic, but not from S.N.Http. QuicException is a public API of S.N.Quic. But we're talking 7.0 time frame here.

Anyway, what I was trying to say/suggest is that we should cover all the result codes eventually. However, this is perfectly good enough for the problem we have here.

{
switch (status)
{
case MsQuicStatusCodes.ConnectionRefused:
return (int)SocketError.ConnectionRefused; // 0x8007274D - WSAECONNREFUSED
case MsQuicStatusCodes.ConnectionTimeout:
return (int)SocketError.TimedOut; // 0x8007274C - WSAETIMEDOUT
case MsQuicStatusCodes.HostUnreachable:
return (int)SocketError.HostUnreachable;
default:
return 0;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,12 @@ public MsQuicConnection(IPEndPoint localEndPoint, IPEndPoint remoteEndPoint, Saf
// constructor for outbound connections
public MsQuicConnection(QuicClientConnectionOptions options)
{
_remoteEndPoint = options.RemoteEndPoint!;
if (options.RemoteEndPoint == null)
{
throw new ArgumentNullException(nameof(options.RemoteEndPoint));
}

_remoteEndPoint = options.RemoteEndPoint;
_configuration = SafeMsQuicConfigurationHandle.Create(options);
_state.RemoteCertificateRequired = true;
if (options.ClientAuthenticationOptions != null)
Expand Down Expand Up @@ -523,13 +528,21 @@ internal override ValueTask WaitForAvailableBidirectionalStreamsAsync(Cancellati
internal override QuicStreamProvider OpenUnidirectionalStream()
{
ThrowIfDisposed();
if (!Connected)
{
throw new InvalidOperationException(SR.net_quic_not_connected);
}

return new MsQuicStream(_state, QUIC_STREAM_OPEN_FLAGS.UNIDIRECTIONAL);
}

internal override QuicStreamProvider OpenBidirectionalStream()
{
ThrowIfDisposed();
if (!Connected)
{
throw new InvalidOperationException(SR.net_quic_not_connected);
}

return new MsQuicStream(_state, QUIC_STREAM_OPEN_FLAGS.NONE);
}
Expand All @@ -552,15 +565,15 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d

if (_configuration is null)
{
throw new Exception($"{nameof(ConnectAsync)} must not be called on a connection obtained from a listener.");
throw new InvalidOperationException($"{nameof(ConnectAsync)} must not be called on a connection obtained from a listener.");
}

QUIC_ADDRESS_FAMILY af = _remoteEndPoint.AddressFamily switch
{
AddressFamily.Unspecified => QUIC_ADDRESS_FAMILY.UNSPEC,
AddressFamily.InterNetwork => QUIC_ADDRESS_FAMILY.INET,
AddressFamily.InterNetworkV6 => QUIC_ADDRESS_FAMILY.INET6,
_ => throw new Exception(SR.Format(SR.net_quic_unsupported_address_family, _remoteEndPoint.AddressFamily))
_ => throw new ArgumentException(SR.Format(SR.net_quic_unsupported_address_family, _remoteEndPoint.AddressFamily))
};

Debug.Assert(_state.StateGCHandle.IsAllocated);
Expand Down Expand Up @@ -592,7 +605,7 @@ internal override ValueTask ConnectAsync(CancellationToken cancellationToken = d
}
else
{
throw new Exception($"Unsupported remote endpoint type '{_remoteEndPoint.GetType()}'.");
throw new ArgumentException($"Unsupported remote endpoint type '{_remoteEndPoint.GetType()}'.");
}

// We store TCS to local variable to avoid NRE if callbacks finish fast and set _state.ConnectTcs to null.
Expand Down Expand Up @@ -759,7 +772,7 @@ private void Dispose(bool disposing)
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(_state, $"{TraceId()} Connection disposing {disposing}");

// If we haven't already shutdown gracefully (via a successful CloseAsync call), then force an abortive shutdown.
if (_state.Handle != null)
if (_state.Handle != null && !_state.Handle.IsInvalid && !_state.Handle.IsClosed)
{
// Handle can be null if outbound constructor failed and we are called from finalizer.
Debug.Assert(!Monitor.IsEntered(_state));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
using System.Threading.Channels;
using System.Threading.Tasks;
using static System.Net.Quic.Implementations.MsQuic.Internal.MsQuicNativeMethods;
using System.Security.Authentication;

namespace System.Net.Quic.Implementations.MsQuic
{
Expand Down Expand Up @@ -219,15 +218,14 @@ private static unsafe uint NativeCallbackHandler(
IntPtr context,
ref ListenerEvent evt)
{
if (evt.Type != QUIC_LISTENER_EVENT.NEW_CONNECTION)
{
return MsQuicStatusCodes.InternalError;
}

GCHandle gcHandle = GCHandle.FromIntPtr(context);
Debug.Assert(gcHandle.IsAllocated);
Debug.Assert(gcHandle.Target is not null);
var state = (State)gcHandle.Target;
if (evt.Type != QUIC_LISTENER_EVENT.NEW_CONNECTION)
{
return MsQuicStatusCodes.InternalError;
}

SafeMsQuicConnectionHandle? connectionHandle = null;
MsQuicConnection? msQuicConnection = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,11 @@ public QuicException(string? message, Exception? innerException)
: base(message, innerException)
{
}

public QuicException(string? message, Exception? innerException, int result)
: base(message, innerException)
{
HResult = result;
}
}
}
Loading