Conversation
Capture isolated request, connection, pool, GOAWAY and retry-path chronology without changing cookie assertions or retry semantics. Reuse opt-in HTTP/3 loopback logging from dotnet#134413. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 4 pipeline(s). 12 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.
Copilot review overview
🟡 Changes recommended
Unresolved diagnostic correctness issues could omit or misorder failure evidence.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds opt-in, isolated HTTP/3 diagnostics for cookie redirect failures without changing functional behavior.
Changes:
- Adds retry, pooling, GOAWAY, QUIC, and lifecycle tracing.
- Runs diagnostics in an isolated process with bounded event capture.
- Adds loopback server and client phase logging.
| File | Summary |
|---|---|
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs |
Captures and filters diagnostic events. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs |
Logs retry paths. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs |
Logs connection, GOAWAY, and shutdown state. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs |
Logs pool publication and invalidation. |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.cs |
Adds retry correlation diagnostics. |
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Cookies.cs |
Adds client and server phase logging. |
src/libraries/Common/tests/System/Net/Http/Http3LoopbackServer.cs |
Adds server lifecycle logging. |
src/libraries/Common/tests/System/Net/Http/Http3LoopbackConnection.cs |
Adds connection and stream diagnostics. |
Comment on lines
+6049
to
+6059
| var events = new ConcurrentQueue<string>(); | ||
| long started = Stopwatch.GetTimestamp(); | ||
| int eventCount = 0; | ||
| void Log(string message) | ||
| { | ||
| int sequence = Interlocked.Increment(ref eventCount); | ||
| if (sequence <= MaximumEvents) | ||
| { | ||
| events.Enqueue($"{sequence}: {Stopwatch.GetElapsedTime(started).TotalMilliseconds:F3}ms thread={Environment.CurrentManagedThreadId} {message}"); | ||
| } | ||
| } |
Comment on lines
+6081
to
+6085
| foreach (string entry in events.ToArray()) | ||
| { | ||
| _output.WriteLine(entry); | ||
| } | ||
| _output.WriteLine($"Diagnostic events beyond limit: {Math.Max(0, Volatile.Read(ref eventCount) - MaximumEvents)}"); |
This branch has not been deployed
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.
Purpose
Experimental diagnostics-only PR for #133842, intended for CI evidence gathering, not a fix or a merge candidate. Please apply/retain
NO-REVIEW.The reported cookie-redirect failures have five distinct final throw paths. This captures enough chronology to distinguish repeated rejected connection reuse from a pool-publication/shutdown race without assuming either hypothesis is the cause.
Diagnostics
RemoteExecutorchild to capture private HTTP/QUIC events without enabling process-wide tracing alongside unrelated tests. Unsupported platforms still execute the original scenario with phase-only logging; they are not skipped.H3_REQUEST_REJECTED, no outbound request stream (including any suppressed exception), stream at/above the GOAWAY boundary, localOperationAborted, and internal cancellation. Preserve retry-count and exception evidence rather than inferring repeated 267 errors from the final message.The two opt-in loopback helper changes are identical to the published helper changes in #134413 at
ebcfc82faa036155193841484e2f9eae8ee8fb56. Its request-reuse test is untouched; overlap is deliberate and no common root cause is asserted.Validation
Windows x64, CoreCLR Release/libraries Debug, local
.NET 12.0.0-dev:build.cmd clr+libs -rc releasebaseline and System.Net.Http rebuild: passed, no warnings/errors.2.5.10.154561281 (9ff06b71fd4b4d5258361598ada5b24cbc1beb20).A small BenchmarkDotNet observer-overhead check on the same Debug library binaries measured 28.57 ms with phase-only logging and 29.60 ms with client tracing (5 iterations, 16 invocations/iteration, 2 warmups; in-process, existing power plan). This includes test setup/exchange/cleanup but excludes child startup/stdout forwarding. It is not a baseline-versus-patch production performance comparison.
Limitations
Tracing and process isolation perturb scheduling. Local Windows passes do not reproduce or exonerate the Linux CI failures. A hard child-process kill/crash before
finallyloses buffered events; ordinary failures and managed factory timeouts flush them. Synthetic cookie assertion failures may themselves include expected/actual test-cookie strings. No retry/suppression workaround or CI rerun is included.Related investigation: #133842.
Note
This PR description and implementation were generated with GitHub Copilot.