From a90fa476c52f2998d08e6f4159e2b71af7e78351 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Wed, 3 Jul 2024 18:59:41 +0200 Subject: [PATCH 1/2] Fixed some tests leaking unobserved exceptions --- .../MsQuicCipherSuitesPolicyTests.cs | 12 +++-- .../tests/FunctionalTests/MsQuicTests.cs | 8 +++- .../FunctionalTests/QuicListenerTests.cs | 3 +- .../tests/FunctionalTests/QuicStreamTests.cs | 46 ++++++++++--------- .../FunctionalTests/QuicTestCollection.cs | 38 +++++++++++---- 5 files changed, 70 insertions(+), 37 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs index 8b15670ee88a2..33f35bfccd0cd 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; using System.Net.Security; +using System.Security.Authentication; using System.Threading.Tasks; using Xunit; using Xunit.Abstractions; @@ -48,7 +49,7 @@ public Task SupportedCipher_Success() [Theory] [InlineData(new TlsCipherSuite[] { })] [InlineData(new[] { TlsCipherSuite.TLS_AES_128_CCM_8_SHA256 })] - public void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) + public async void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) { CipherSuitesPolicy policy = new CipherSuitesPolicy(ciphers); var listenerOptions = new QuicListenerOptions() @@ -62,11 +63,14 @@ public void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) return ValueTask.FromResult(serverOptions); } }; - Assert.ThrowsAsync(async () => await CreateQuicListener(listenerOptions)); + await using var listener = await CreateQuicListener(listenerOptions); - var clientOptions = CreateQuicClientOptions(new IPEndPoint(IPAddress.Loopback, 5000)); + var clientOptions = CreateQuicClientOptions(listener.LocalEndPoint); clientOptions.ClientAuthenticationOptions.CipherSuitesPolicy = policy; - Assert.ThrowsAsync(async () => await CreateQuicConnection(clientOptions)); + await Assert.ThrowsAsync(async () => await CreateQuicConnection(clientOptions)); + + await Assert.ThrowsAsync(async () => await CreateQuicConnection(listener.LocalEndPoint)); + await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); } [Fact] diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 41ac5e41da24d..f7bb1517ef340 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -153,7 +153,8 @@ public async Task QuicRootedConnectionGetsReleased_ConnectFails() }; QuicListener listener = await CreateQuicListener(listenerOptions); - await Assert.ThrowsAsync(async () => await CreateConnectedQuicConnection(listener)); + await Assert.ThrowsAsync(async () => await CreateQuicConnection(listener.LocalEndPoint)); + await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); // Dispose everything and check if all weak references are dead. await listener.DisposeAsync(); @@ -877,10 +878,13 @@ public async Task OpenStreamAsync_BlocksUntilAvailable_PeerClosesWritingUnidirec // Open one stream, second call should block await using var stream = await clientConnection.OpenOutboundStreamAsync(QuicStreamType.Unidirectional); await stream.WriteAsync(new byte[64 * 1024], completeWrites: true); - await Assert.ThrowsAsync(() => clientConnection.OpenOutboundStreamAsync(QuicStreamType.Unidirectional).AsTask().WaitAsync(TimeSpan.FromSeconds(1))); + var pendingOpenStreamTask = clientConnection.OpenOutboundStreamAsync(QuicStreamType.Unidirectional); + Assert.False(pendingOpenStreamTask.IsCompleted); await clientConnection.DisposeAsync(); await serverConnection.DisposeAsync(); + + await Assert.ThrowsAsync(async () => await pendingOpenStreamTask); } [Theory] diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs index 6e3971764d18f..4f240d75dead8 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; @@ -73,6 +73,7 @@ public async Task AcceptConnectionAsync_InvalidConnectionOptions_Throws() ValueTask connectTask = CreateQuicConnection(listener.LocalEndPoint); await Assert.ThrowsAnyAsync(async () => await listener.AcceptConnectionAsync()); + await connectTask; } [Fact] diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs index 8f455d61f6a7c..d40d44400bf31 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs @@ -1452,7 +1452,7 @@ await RunClientServer( } else { - await WaitingSide(stream, tcs.Task, DefaultStreamErrorCodeClient); + await WaitingSide(stream, tcs.Task, true); } }, clientFunction: async connection => @@ -1465,7 +1465,7 @@ await RunClientServer( if (disposeServer) { - await WaitingSide(stream, tcs.Task, DefaultStreamErrorCodeServer); + await WaitingSide(stream, tcs.Task, false); } else { @@ -1493,10 +1493,6 @@ async ValueTask DisposeSide(QuicStream stream, TaskCompletionSource tcs) await stream.DisposeAsync(); - // Reads should be aborted as we didn't consume the data. - var readEx = await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, () => stream.ReadsClosed); - Assert.Null(readEx.ApplicationErrorCode); - // Writes should be aborted as we aborted them. if (abortCode.HasValue) { @@ -1509,29 +1505,21 @@ async ValueTask DisposeSide(QuicStream stream, TaskCompletionSource tcs) Assert.True(stream.WritesClosed.IsCompletedSuccessfully); } + // Reads should be aborted as we didn't consume the data. + var readEx = await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, () => stream.ReadsClosed); + Assert.Null(readEx.ApplicationErrorCode); + tcs.SetResult(abortCode); } - async ValueTask WaitingSide(QuicStream stream, Task task, long errorCode) + async ValueTask WaitingSide(QuicStream stream, Task task, bool server) { long? abortCode = await task; - // Reads will be aborted by the peer as we didn't consume them all. - if (abortCode.HasValue) - { - var readEx = await AssertThrowsQuicExceptionAsync(QuicError.StreamAborted, () => stream.ReadsClosed); - Assert.Equal(abortCode.Value, readEx.ApplicationErrorCode); - } - // Reads should be still open as the peer closed gracefully and we are keeping the data in buffer. - else - { - Assert.False(stream.ReadsClosed.IsCompleted); - } - if (!completeWrites) { // Writes must be aborted by the peer as we didn't complete them. var writeEx = await AssertThrowsQuicExceptionAsync(QuicError.StreamAborted, () => stream.WritesClosed); - Assert.Equal(errorCode, writeEx.ApplicationErrorCode); + Assert.Equal(server ? DefaultStreamErrorCodeClient : DefaultStreamErrorCodeServer, writeEx.ApplicationErrorCode); } else { @@ -1545,9 +1533,25 @@ async ValueTask WaitingSide(QuicStream stream, Task task, long errorCode) { QuicException qe = Assert.IsType(ex); Assert.Equal(QuicError.StreamAborted, qe.QuicError); - Assert.Equal(errorCode, qe.ApplicationErrorCode); + Assert.Equal(server ? DefaultStreamErrorCodeClient : DefaultStreamErrorCodeServer, qe.ApplicationErrorCode); } } + + // Reads will be aborted by the peer as we didn't consume them all. + if (abortCode.HasValue) + { + var readEx = await AssertThrowsQuicExceptionAsync(QuicError.StreamAborted, () => stream.ReadsClosed); + Assert.Equal(abortCode.Value, readEx.ApplicationErrorCode); + } + // Reads should be still open as the peer closed gracefully and we are keeping the data in buffer. + else + { + Assert.False(stream.ReadsClosed.IsCompleted); + + // Dispose will abort reading from this side. + await stream.DisposeAsync(); + var readEx = await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, () => stream.ReadsClosed); + } } } } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs index 5125c72e0ca8d..db7c8de91d2e4 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs @@ -1,16 +1,15 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using Microsoft.DotNet.XUnitExtensions; -using System; -using System.Net.Quic; +using Microsoft.Quic; using System.Reflection; using System.Text; using System.Linq; +using System.Threading.Tasks; +using System.Threading; +using System.Collections.Generic; using Xunit; -using Xunit.Abstractions; -using Microsoft.Quic; using static Microsoft.Quic.MsQuic; namespace System.Net.Quic.Tests; @@ -22,6 +21,8 @@ public unsafe class QuicTestCollection : ICollectionFixture, public static Version MsQuicVersion { get; } = GetMsQuicVersion(); + private static readonly Dictionary s_unobservedExceptions = new Dictionary(); + public QuicTestCollection() { string msQuicLibraryVersion = GetMsQuicLibraryVersion(); @@ -33,11 +34,28 @@ public QuicTestCollection() QUIC_SETTINGS settings = default(QUIC_SETTINGS); settings.IsSet.MaxWorkerQueueDelayUs = 1; settings.MaxWorkerQueueDelayUs = 2_500_000u; // 2.5s, 10x the default - if (MsQuic.StatusFailed(GetApiTable()->SetParam(null, MsQuic.QUIC_PARAM_GLOBAL_SETTINGS, (uint)sizeof(QUIC_SETTINGS), (byte*)&settings))) + if (StatusFailed(GetApiTable()->SetParam(null, QUIC_PARAM_GLOBAL_SETTINGS, (uint)sizeof(QUIC_SETTINGS), (byte*)&settings))) { Console.WriteLine($"Unable to set MsQuic MaxWorkerQueueDelayUs."); } } + + EventHandler eventHandler = (_, e) => + { + lock (s_unobservedExceptions) + { + string text = e.Exception.ToString(); + if (!s_unobservedExceptions.ContainsKey(text)) + { + s_unobservedExceptions[text] = 1; + } + else + { + s_unobservedExceptions[text] += 1; + } + } + }; + TaskScheduler.UnobservedTaskException += eventHandler; } public unsafe void Dispose() @@ -60,7 +78,7 @@ public unsafe void Dispose() if (StatusFailed(status)) { - System.Console.WriteLine($"Failed to read MsQuic counters: {status}"); + Console.WriteLine($"Failed to read MsQuic counters: {status}"); return; } @@ -81,7 +99,9 @@ void DumpCounter(QUIC_PERFORMANCE_COUNTERS counter) DumpCounter(QUIC_PERFORMANCE_COUNTERS.CONN_APP_REJECT); DumpCounter(QUIC_PERFORMANCE_COUNTERS.CONN_LOAD_REJECT); - System.Console.WriteLine(sb.ToString()); + Console.WriteLine(sb.ToString()); + + Console.WriteLine($"Unobserved exceptions of {s_unobservedExceptions.Count} different types: {Environment.NewLine}{string.Join(Environment.NewLine + new string('=', 120) + Environment.NewLine, s_unobservedExceptions.Select(pair => $"Count {pair.Value}: {pair.Key}"))}"); } private static Version GetMsQuicVersion() @@ -102,6 +122,6 @@ private static Version GetMsQuicVersion() { Type msQuicApiType = Type.GetType("System.Net.Quic.MsQuicApi, System.Net.Quic"); object msQuicApiInstance = msQuicApiType.GetProperty("Api", BindingFlags.NonPublic | BindingFlags.Static).GetGetMethod(true).Invoke(null, Array.Empty()); - return (QUIC_API_TABLE*)(Pointer.Unbox(msQuicApiType.GetProperty("ApiTable").GetGetMethod().Invoke(msQuicApiInstance, Array.Empty()))); + return (QUIC_API_TABLE*)Pointer.Unbox(msQuicApiType.GetProperty("ApiTable").GetGetMethod().Invoke(msQuicApiInstance, Array.Empty())); } } From 546f42192c56603561dac4d1239ed649ec1856dd Mon Sep 17 00:00:00 2001 From: ManickaP Date: Mon, 8 Jul 2024 11:37:14 +0200 Subject: [PATCH 2/2] Feedback --- .../FunctionalTests/MsQuicCipherSuitesPolicyTests.cs | 4 +++- .../tests/FunctionalTests/QuicListenerTests.cs | 2 +- .../tests/FunctionalTests/QuicStreamTests.cs | 2 +- .../tests/FunctionalTests/QuicTestCollection.cs | 9 +-------- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs index 33f35bfccd0cd..333be4e4a8138 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs @@ -49,7 +49,7 @@ public Task SupportedCipher_Success() [Theory] [InlineData(new TlsCipherSuite[] { })] [InlineData(new[] { TlsCipherSuite.TLS_AES_128_CCM_8_SHA256 })] - public async void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) + public async Task NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) { CipherSuitesPolicy policy = new CipherSuitesPolicy(ciphers); var listenerOptions = new QuicListenerOptions() @@ -65,10 +65,12 @@ public async void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ci }; await using var listener = await CreateQuicListener(listenerOptions); + // Creating a connection with incompatible ciphers. var clientOptions = CreateQuicClientOptions(listener.LocalEndPoint); clientOptions.ClientAuthenticationOptions.CipherSuitesPolicy = policy; await Assert.ThrowsAsync(async () => await CreateQuicConnection(clientOptions)); + // Creating a connection to a server configured with incompatible ciphers. await Assert.ThrowsAsync(async () => await CreateQuicConnection(listener.LocalEndPoint)); await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs index 4f240d75dead8..c65b38c73c746 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs @@ -73,7 +73,7 @@ public async Task AcceptConnectionAsync_InvalidConnectionOptions_Throws() ValueTask connectTask = CreateQuicConnection(listener.LocalEndPoint); await Assert.ThrowsAnyAsync(async () => await listener.AcceptConnectionAsync()); - await connectTask; + await Assert.ThrowsAnyAsync(async () => await connectTask); } [Fact] diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs index d40d44400bf31..164a4817a2fe3 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs @@ -1550,7 +1550,7 @@ async ValueTask WaitingSide(QuicStream stream, Task task, bool server) // Dispose will abort reading from this side. await stream.DisposeAsync(); - var readEx = await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, () => stream.ReadsClosed); + await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, () => stream.ReadsClosed); } } } diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs index db7c8de91d2e4..699a41fcd1042 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs @@ -45,14 +45,7 @@ public QuicTestCollection() lock (s_unobservedExceptions) { string text = e.Exception.ToString(); - if (!s_unobservedExceptions.ContainsKey(text)) - { - s_unobservedExceptions[text] = 1; - } - else - { - s_unobservedExceptions[text] += 1; - } + s_unobservedExceptions[text] = s_unobservedExceptions.GetValueOrDefault(text) + 1; } }; TaskScheduler.UnobservedTaskException += eventHandler;