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] Fixed some tests leaking unobserved exceptions #104444

Merged
merged 2 commits into from
Jul 9, 2024
Merged

[QUIC] Fixed some tests leaking unobserved exceptions #104444

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a90fa47
Fixed some tests leaking unobserved exceptions
ManickaP Jul 3, 2024
546f421
Feedback
ManickaP Jul 8, 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
12 changes: 8 additions & 4 deletions src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
ManickaP marked this conversation as resolved.
Show resolved Hide resolved
{
CipherSuitesPolicy policy = new CipherSuitesPolicy(ciphers);
var listenerOptions = new QuicListenerOptions()
Expand All @@ -62,11 +63,14 @@ public void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers)
return ValueTask.FromResult(serverOptions);
}
};
Assert.ThrowsAsync<ArgumentException>(async () => await CreateQuicListener(listenerOptions));
await using var listener = await CreateQuicListener(listenerOptions);
Copy link
Member

Choose a reason for hiding this comment

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

How come this was supposed to be throwing before?

Copy link
Member

Choose a reason for hiding this comment

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

it is possible it was never throwing and the test was wrong, the Tasks were not awaited in this test at all before this PR

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, the QuicListener creation should not throw, because we don't check the CipherSuitesPolicy there, we only access it when we get an incoming connection.


var clientOptions = CreateQuicClientOptions(new IPEndPoint(IPAddress.Loopback, 5000));
var clientOptions = CreateQuicClientOptions(listener.LocalEndPoint);
clientOptions.ClientAuthenticationOptions.CipherSuitesPolicy = policy;
Assert.ThrowsAsync<ArgumentException>(async () => await CreateQuicConnection(clientOptions));
await Assert.ThrowsAsync<ArgumentException>(async () => await CreateQuicConnection(clientOptions));

await Assert.ThrowsAsync<AuthenticationException>(async () => await CreateQuicConnection(listener.LocalEndPoint));
await Assert.ThrowsAsync<ArgumentException>(async () => await listener.AcceptConnectionAsync());
ManickaP marked this conversation as resolved.
Show resolved Hide resolved
}

[Fact]
Expand Down
8 changes: 6 additions & 2 deletions src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ public async Task QuicRootedConnectionGetsReleased_ConnectFails()
};
QuicListener listener = await CreateQuicListener(listenerOptions);

await Assert.ThrowsAsync<AuthenticationException>(async () => await CreateConnectedQuicConnection(listener));
await Assert.ThrowsAsync<AuthenticationException>(async () => await CreateQuicConnection(listener.LocalEndPoint));
await Assert.ThrowsAsync<ArgumentException>(async () => await listener.AcceptConnectionAsync());

// Dispose everything and check if all weak references are dead.
await listener.DisposeAsync();
Expand Down Expand Up @@ -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<TimeoutException>(() => 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<ObjectDisposedException>(async () => await pendingOpenStreamTask);
}

[Theory]
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -73,6 +73,7 @@ public async Task AcceptConnectionAsync_InvalidConnectionOptions_Throws()

ValueTask<QuicConnection> connectTask = CreateQuicConnection(listener.LocalEndPoint);
await Assert.ThrowsAnyAsync<ArgumentException>(async () => await listener.AcceptConnectionAsync());
await connectTask;
}

[Fact]
Expand Down
46 changes: 25 additions & 21 deletions src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1452,7 +1452,7 @@ await RunClientServer(
}
else
{
await WaitingSide(stream, tcs.Task, DefaultStreamErrorCodeClient);
await WaitingSide(stream, tcs.Task, true);
}
},
clientFunction: async connection =>
Expand All @@ -1465,7 +1465,7 @@ await RunClientServer(

if (disposeServer)
{
await WaitingSide(stream, tcs.Task, DefaultStreamErrorCodeServer);
await WaitingSide(stream, tcs.Task, false);
}
else
{
Expand Down Expand Up @@ -1493,10 +1493,6 @@ async ValueTask DisposeSide(QuicStream stream, TaskCompletionSource<long?> 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)
{
Expand All @@ -1509,29 +1505,21 @@ async ValueTask DisposeSide(QuicStream stream, TaskCompletionSource<long?> 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<long?> task, long errorCode)
async ValueTask WaitingSide(QuicStream stream, Task<long?> 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
{
Expand All @@ -1545,9 +1533,25 @@ async ValueTask WaitingSide(QuicStream stream, Task<long?> task, long errorCode)
{
QuicException qe = Assert.IsType<QuicException>(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);
ManickaP marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
Expand Down
38 changes: 29 additions & 9 deletions src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -22,6 +21,8 @@ public unsafe class QuicTestCollection : ICollectionFixture<QuicTestCollection>,

public static Version MsQuicVersion { get; } = GetMsQuicVersion();

private static readonly Dictionary<string, int> s_unobservedExceptions = new Dictionary<string, int>();

public QuicTestCollection()
{
string msQuicLibraryVersion = GetMsQuicLibraryVersion();
Expand All @@ -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<UnobservedTaskExceptionEventArgs> eventHandler = (_, e) =>
{
lock (s_unobservedExceptions)
{
string text = e.Exception.ToString();
if (!s_unobservedExceptions.ContainsKey(text))
{
s_unobservedExceptions[text] = 1;
}
else
{
s_unobservedExceptions[text] += 1;
}
ManickaP marked this conversation as resolved.
Show resolved Hide resolved
}
};
TaskScheduler.UnobservedTaskException += eventHandler;
}

public unsafe void Dispose()
Expand All @@ -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;
}

Expand All @@ -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()
Expand All @@ -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<object?>());
return (QUIC_API_TABLE*)(Pointer.Unbox(msQuicApiType.GetProperty("ApiTable").GetGetMethod().Invoke(msQuicApiInstance, Array.Empty<object?>())));
return (QUIC_API_TABLE*)Pointer.Unbox(msQuicApiType.GetProperty("ApiTable").GetGetMethod().Invoke(msQuicApiInstance, Array.Empty<object?>()));
}
}
Loading