[QUIC] fix unobserved exceptions #104894
[QUIC] fix unobserved exceptions #104894
Conversation
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl |
| } | ||
|
|
||
| internal static bool TryGetStreamExceptionForMsQuicStatus(int status, [NotNullWhen(true)] out Exception? exception, bool streamWasSuccessfullyStarted = true, string? message = null) | ||
| internal static bool TryGetStreamExceptionForMsQuicStatus(int status, [NotNullWhen(true)] out Exception? exception) |
There was a problem hiding this comment.
We don't need the special exception here. MsQuic will throw either ABORTED or INVALID_STATE in case the connection is closed (remotely / locally). It also doesn't use these statuses for anything else.
| } | ||
| } | ||
|
|
||
| public static void ObserveException(this Task task) |
There was a problem hiding this comment.
Follows the same pattern as LogExceptions in HttpConnection.
| /// <summary> | ||
| /// Completed when connection shutdown is initiated. | ||
| /// </summary> | ||
| private readonly TaskCompletionSource _connectionCloseTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); |
There was a problem hiding this comment.
Just moved around to be next to other connection closure fields.
| Debug.Assert(_connectionCloseTcs.Task.IsCompleted); | ||
| _handle.Dispose(); | ||
| _shutdownTokenSource.Dispose(); | ||
| _connectionCloseTcs.Task.ObserveException(); |
I thought the same when I added dumping MsQuic counters for System.Net.Quic tests. But I am not sure we can just go and update xUnit to v3 |
I don't think it's even out yet 😄 |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
3 participants
Addresses the last issue with unobserved exception leaking from
_connectionCloseTcs.Fixes #80111
Testing
I used Miha's trick from #80111 (comment) to manually check for unobserved exceptions from HTTP tests, but we don't have a collection fixture like
QuicTestCollectionto add similar print like:runtime/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs
Line 97 in 9d24b13
in HTTP. And adding it and annotating the whole test lib felt like out of the scope for this PR.
Also note that from xUnit 3 there should be
Assembly Fixtureswhich wouldn't require annotating every single test class in an assembly.