[QUIC] Listener AcceptConnectionAsync propagates errors from failed h…

…andshake (#72390)

* Propagate exception from handshake to the caller of AcceptConnectionAsync

* ThrowsAnyAsync -> ThrowsAsync for QuicException since there's no hierarchy anymore

* Added tests

* Feedback

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs

@@ -28,15 +28,15 @@ private sealed class PendingConnection : IAsyncDisposable
         /// <summary>
         /// It will contain the established <see cref="QuicConnection" /> in case of a successful handshake; otherwise, <c>null</c>.
         /// </summary>
-        private readonly TaskCompletionSource<QuicConnection?> _finishHandshakeTask;
+        private readonly TaskCompletionSource<QuicConnection> _finishHandshakeTask;
         /// <summary>
         /// Use to impose the handshake timeout.
         /// </summary>
         private readonly CancellationTokenSource _cancellationTokenSource;
 
         public PendingConnection()
         {
-            _finishHandshakeTask = new TaskCompletionSource<QuicConnection?>(TaskCreationOptions.RunContinuationsAsynchronously);
+            _finishHandshakeTask = new TaskCompletionSource<QuicConnection>(TaskCreationOptions.RunContinuationsAsynchronously);
             _cancellationTokenSource = new CancellationTokenSource();
         }
 
@@ -74,7 +74,7 @@ public async void StartHandshake(QuicConnection connection, SslClientHelloInfo c
 
                 await connection.CloseAsync(default).ConfigureAwait(false);
                 await connection.DisposeAsync().ConfigureAwait(false);
-                _finishHandshakeTask.SetResult(null);
+                _finishHandshakeTask.SetException(ex);
             }
         }
 
@@ -83,18 +83,25 @@ public async void StartHandshake(QuicConnection connection, SslClientHelloInfo c
         /// </summary>
         /// <param name="cancellationToken">A cancellation token that can be used to cancel the asynchronous operation.</param>
         /// <returns>An asynchronous task that completes with the established connection if it succeeded or <c>null</c> if it failed.</returns>
-        public ValueTask<QuicConnection?> FinishHandshakeAsync(CancellationToken cancellationToken = default)
+        public ValueTask<QuicConnection> FinishHandshakeAsync(CancellationToken cancellationToken = default)
             => new(_finishHandshakeTask.Task.WaitAsync(cancellationToken));
 
 
         /// <summary>
         /// Cancels the handshake in progress and awaits for it so that the connection can be safely cleaned from the listener queue.
         /// </summary>
         /// <returns>A task that represents the asynchronous dispose operation.</returns>
-        public ValueTask DisposeAsync()
+        public async ValueTask DisposeAsync()
         {
             _cancellationTokenSource.Cancel();
-            return new ValueTask(_finishHandshakeTask.Task);
+            try
+            {
+                await _finishHandshakeTask.Task.ConfigureAwait(false);
+            }
+            catch
+            {
+                // Just swallow the exception, we don't want it to propagate outside of dispose and it has already been logged in StartHandshake catch block.
+            }
         }
     }
 }

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs

@@ -66,7 +66,7 @@ public static ValueTask<QuicListener> ListenAsync(QuicListenerOptions options, C
     /// <summary>
     /// Handle to MsQuic listener object.
     /// </summary>
-    private MsQuicContextSafeHandle _handle;
+    private readonly MsQuicContextSafeHandle _handle;
 
     /// <summary>
     /// Set to non-zero once disposed. Prevents double and/or concurrent disposal.
@@ -153,6 +153,9 @@ private unsafe QuicListener(QuicListenerOptions options)
     /// <remarks>
     /// Note that <see cref="QuicListener" /> doesn't have a mechanism to report inbound connections that fail the handshake process.
     /// Such connections are only logged by the listener and never surfaced on the outside.
+    ///
+    /// Propagates exceptions from <see cref="QuicListenerOptions.ConnectionOptionsCallback"/>, including validation errors from misconfigured <see cref="QuicServerConnectionOptions"/>, e.g. <see cref="ArgumentException"/>.
+    /// Also propagates exceptions from failed connection handshake, e.g. <see cref="AuthenticationException"/>, <see cref="QuicException"/>.
     /// </remarks>
     /// <returns>A task that will contain a fully connected <see cref="QuicConnection" /> which successfully finished the handshake and is ready to be used.</returns>
     public async ValueTask<QuicConnection> AcceptConnectionAsync(CancellationToken cancellationToken = default)
@@ -161,20 +164,10 @@ public async ValueTask<QuicConnection> AcceptConnectionAsync(CancellationToken c
 
         try
         {
-            while (true)
+            PendingConnection pendingConnection = await _acceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false);
+            await using (pendingConnection.ConfigureAwait(false))
             {
-                PendingConnection pendingConnection = await _acceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false);
-                await using (pendingConnection.ConfigureAwait(false))
-                {
-                    QuicConnection? connection = await pendingConnection.FinishHandshakeAsync(cancellationToken).ConfigureAwait(false);
-                    // Handshake failed, discard this connection and try to get another from the queue.
-                    if (connection is null)
-                    {
-                        continue;
-                    }
-
-                    return connection;
-                }
+                return await pendingConnection.FinishHandshakeAsync(cancellationToken).ConfigureAwait(false);
             }
         }
         catch (ChannelClosedException ex) when (ex.InnerException is not null)

src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs

@@ -56,7 +56,7 @@ public sealed partial class QuicStream
     /// <summary>
     /// Handle to MsQuic connection object.
     /// </summary>
-    private MsQuicContextSafeHandle _handle;
+    private readonly MsQuicContextSafeHandle _handle;
 
     /// <summary>
     /// Set to non-zero once disposed. Prevents double and/or concurrent disposal.

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs

@@ -71,7 +71,7 @@ public void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers)
         [Fact]
         public async Task MismatchedCipherPolicies_ConnectAsync_ThrowsQuicException()
         {
-            await Assert.ThrowsAnyAsync<QuicException>(() => TestConnection(
+            await Assert.ThrowsAsync<QuicException>(() => TestConnection(
                new CipherSuitesPolicy(new[] { TlsCipherSuite.TLS_AES_128_GCM_SHA256 }),
                new CipherSuitesPolicy(new[] { TlsCipherSuite.TLS_AES_256_GCM_SHA384 })
             ));

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs

@@ -132,12 +132,11 @@ public async Task UntrustedClientCertificateFails()
             clientOptions.ClientAuthenticationOptions.ClientCertificates = new X509CertificateCollection() { ClientCertificate };
             Task<QuicConnection> clientTask = CreateQuicConnection(clientOptions).AsTask();
 
-            using CancellationTokenSource cts = new CancellationTokenSource();
-            cts.CancelAfter(500); //Some delay to see if we would get failed connection.
-            Task<QuicConnection> serverTask = listener.AcceptConnectionAsync(cts.Token).AsTask();
+            // This will propagate the AuthenticationException since the client certificate is not trusted.
+            Task<QuicConnection> serverTask = listener.AcceptConnectionAsync().AsTask();
 
             Assert.True(clientTask.Wait(PassingTestTimeout));
-            await Assert.ThrowsAsync<OperationCanceledException>(() => serverTask);
+            await Assert.ThrowsAsync<AuthenticationException>(() => serverTask);
             // The task will likely succeed but we don't really care.
             // It may fail if the server aborts quickly.
             try
@@ -179,6 +178,7 @@ public async Task CertificateCallbackThrowPropagates()
             clientOptions.ClientAuthenticationOptions.TargetHost = "foobar1";
 
             await Assert.ThrowsAsync<ArithmeticException>(() => CreateQuicConnection(clientOptions).AsTask());
+            await Assert.ThrowsAsync<AuthenticationException>(async () => await listener.AcceptConnectionAsync());
 
             // Make sure the listener is still usable and there is no lingering bad connection
             validationResult = true;
@@ -245,8 +245,9 @@ public async Task ConnectWithServerCertificateCallback()
             clientOptions.ClientAuthenticationOptions.TargetHost = "foobar3";
             Task clientTask = CreateQuicConnection(clientOptions).AsTask();
 
-            await Assert.ThrowsAnyAsync<QuicException>(() => clientTask);
+            await Assert.ThrowsAsync<QuicException>(() => clientTask);
             Assert.Equal(clientOptions.ClientAuthenticationOptions.TargetHost, receivedHostName);
+            await Assert.ThrowsAsync<ArgumentException>(async () => await listener.AcceptConnectionAsync());
 
             // Do this last to make sure Listener is still functional.
             clientOptions.ClientAuthenticationOptions.TargetHost = "foobar2";
@@ -599,7 +600,7 @@ ValueTask<QuicStream> OpenStreamAsync(QuicConnection connection, CancellationTok
                 // (CloseAsync may be processed before OpenStreamAsync as it is scheduled to the front of the operation queue)
                 // To be revisited once we standartize on exceptions.
                 // [ActiveIssue("https://github.com/dotnet/runtime/issues/55619")]
-                await Assert.ThrowsAnyAsync<QuicException>(() => waitTask.AsTask().WaitAsync(TimeSpan.FromSeconds(3)));
+                await Assert.ThrowsAsync<QuicException>(() => waitTask.AsTask().WaitAsync(TimeSpan.FromSeconds(3)));
             }
             else
             {

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs

@@ -74,12 +74,12 @@ await RunClientServer(
                     // (CloseAsync may be processed before OpenStreamAsync as it is scheduled to the front of the operation queue)
                     // To be revisited once we standartize on exceptions.
                     // [ActiveIssue("https://github.com/dotnet/runtime/issues/55619")]
-                    await Assert.ThrowsAnyAsync<QuicException>(() => connectTask);
+                    await Assert.ThrowsAsync<QuicException>(() => connectTask);
 
                     // Subsequent attempts should fail
                     // TODO: Which exception is correct?
                     await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, async () => await serverConnection.AcceptInboundStreamAsync());
-                    await Assert.ThrowsAnyAsync<QuicException>(() => OpenAndUseStreamAsync(serverConnection));
+                    await Assert.ThrowsAsync<QuicException>(() => OpenAndUseStreamAsync(serverConnection));
                 });
         }
 

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs

@@ -55,5 +55,34 @@ await Task.Run(async () =>
                 await using QuicConnection clientConnection = await clientStreamTask;
             }).WaitAsync(TimeSpan.FromSeconds(6));
         }
+
+        [Fact]
+        public async Task AcceptConnectionAsync_InvalidConnectionOptions_Throws()
+        {
+            QuicListenerOptions listenerOptions = CreateQuicListenerOptions();
+            // Do not set any options, which should throw an argument exception from accept.
+            listenerOptions.ConnectionOptionsCallback = (_, _, _) => ValueTask.FromResult(new QuicServerConnectionOptions());
+            await using QuicListener listener = await CreateQuicListener(listenerOptions);
+
+            ValueTask<QuicConnection> connectTask = CreateQuicConnection(listener.LocalEndPoint);
+            await Assert.ThrowsAnyAsync<ArgumentException>(async () => await listener.AcceptConnectionAsync());
+        }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task AcceptConnectionAsync_ThrowingOptionsCallback_Throws(bool useFromException)
+        {
+            const string expectedMessage = "Expected Message";
+
+            QuicListenerOptions listenerOptions = CreateQuicListenerOptions();
+            // Throw an exception, which should throw the same from accept.
+            listenerOptions.ConnectionOptionsCallback = (_, _, _) => useFromException ? ValueTask.FromException<QuicServerConnectionOptions>(new Exception(expectedMessage)) : throw new Exception(expectedMessage);
+            await using QuicListener listener = await CreateQuicListener(listenerOptions);
+
+            ValueTask<QuicConnection> connectTask = CreateQuicConnection(listener.LocalEndPoint);
+            Exception exception = await Assert.ThrowsAsync<Exception>(async () => await listener.AcceptConnectionAsync());
+            Assert.Equal(expectedMessage, exception.Message);
+        }
     }
 }