[QUIC] Timing fix and more logging - #132012
Open
ManickaP wants to merge 1 commit into
Open
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts System.Net.Quic functional test timing and adds additional diagnostics to help investigate intermittent QUIC test failures (notably around stream-capacity/cancellation and connection-accept behavior).
Changes:
- Updates
GetStreamCapacity_OpenCloseStreamCanceledIntoNegative_CountsCorrectlyto wait for pending stream-open operations to observe cancellation before asserting capacity increments. - Tweaks test data structures (lists → arrays) in the same test to better reflect fixed-size usage.
- Adds server-side accept-task logging in
CreateConnectedQuicConnectionto aid debugging when connection setup fails.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs | Converts server accept to a Task and adds logging around accept completion/failure during connection setup exceptions. |
| src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs | Improves cancellation determinism in the stream-capacity test and adjusts how stream/task collections are materialized. |
Comment on lines
+408
to
+410
| QuicStream[] clientStreams = (await Task.WhenAll(Enumerable.Range(0, unidirectional ? QuicDefaults.DefaultServerMaxInboundUnidirectionalStreams : QuicDefaults.DefaultServerMaxInboundBidirectionalStreams) | ||
| .Select(i => clientConnection.OpenOutboundStreamAsync(unidirectional ? QuicStreamType.Unidirectional : QuicStreamType.Bidirectional).AsTask()))) | ||
| .ToArray(); |
|
|
||
| // Pending streams should get cancelled and disposing the streams now should lead to stream capacity increments. | ||
| bool first = true; // The stream capacity is cumulatively reported only after the STREAMS_AVAILABLE reached over 0. | ||
| OperationCanceledException aex = await Assert.ThrowsAsync<OperationCanceledException>(() => Task.WhenAll(pendingClientStreams)); |
Comment on lines
+253
to
+256
| if (serverTask.IsCompleted) | ||
| { | ||
| _output.WriteLine($"Server {(serverTask.IsCompletedSuccessfully ? "succeeded" : "failed with " + serverTask.Exception)}"); | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs:256
- This logging prints
failed withwhenserverTaskis canceled becauseTask.Exceptionis null for canceled tasks. Make the message handle the canceled case explicitly so failures are diagnosable.
if (serverTask.IsCompleted)
{
_output.WriteLine($"Server {(serverTask.IsCompletedSuccessfully ? "succeeded" : "failed with " + serverTask.Exception)}");
}
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs:435
- The local variable
oceis assigned but never used, which will produce a compiler warning (and in this repo warnings often fail the build). If the exception instance isn't needed, just await the assertion without storing it.
OperationCanceledException oce = await Assert.ThrowsAsync<OperationCanceledException>(() => Task.WhenAll(pendingClientStreams));
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #132000
Contributes to #131950