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 2 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 All @@ -37,6 +36,12 @@ private sealed class State

public QuicOptions ConnectionOptions = new QuicOptions();
public SslServerAuthenticationOptions AuthenticationOptions = new SslServerAuthenticationOptions();
#if DEBUG
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? I'm not a fan of cluttering the product code with debugging helpers.

Copy link
Member Author

Choose a reason for hiding this comment

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

really? => no.

I spent fair amount of time debugging test failures and I was adding and removing various instrumentations wishing it was there.
I work on products in the past where counter were part of diagnostic strategy.
I wish we can keep them for product as well but I'm not ready to push it at this point.

Perhaps @geoffkizer or @stephentoub would have preference.
I can certainly take them out.

Copy link
Member

Choose a reason for hiding this comment

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

We need a third opinion here 😄 @geoffkizer @stephentoub could you chime in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind having stuff like this as long as we think it's generally useful. I think it starts to verge into clutter if it's not super useful and/or not clear what the value is.

I would suggest
(a) Add comments on the field declarations explaining what each of these does
(b) Let's all evaluate each one and agree if we think it's worth keeping or not

public int EventCount;
public int ErrorCount;
public int ConnectionCount;
public Exception? ex;
#endif

public State(QuicListenerOptions options)
{
Expand Down Expand Up @@ -219,15 +224,20 @@ 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 DEBUG
state.EventCount++;
#endif
if (evt.Type != QUIC_LISTENER_EVENT.NEW_CONNECTION)
{
#if DEBUG
state.ErrorCount++;
#endif
return MsQuicStatusCodes.InternalError;
}

SafeMsQuicConnectionHandle? connectionHandle = null;
MsQuicConnection? msQuicConnection = null;
Expand Down Expand Up @@ -266,6 +276,10 @@ private static unsafe uint NativeCallbackHandler(
if (connectionConfiguration == null)
{
// We don't have safe handle yet so MsQuic will cleanup new connection.
#if DEBUG
state.ErrorCount++;
#endif

return MsQuicStatusCodes.InternalError;
}
}
Expand All @@ -280,6 +294,10 @@ private static unsafe uint NativeCallbackHandler(

if (state.AcceptConnectionQueue.Writer.TryWrite(msQuicConnection))
{
#if DEBUG
state.ConnectionCount++;
#endif

return MsQuicStatusCodes.Success;
}
}
Expand All @@ -288,12 +306,18 @@ private static unsafe uint NativeCallbackHandler(
}
catch (Exception ex)
{
#if DEBUG
state.ex = ex;
#endif

if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Error(state, $"[Listener#{state.GetHashCode()}] Exception occurred during handling {(QUIC_LISTENER_EVENT)evt.Type} connection callback: {ex}");
}
}

#if DEBUG
state.ErrorCount++;
#endif
// This handle will be cleaned up by MsQuic by returning InternalError.
connectionHandle?.SetHandleAsInvalid();
msQuicConnection?.Dispose();
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