Avoid early HTTP/2 teardown in WinHTTP Latin1 header test - #133955
Merged
Merged
Conversation
Wait for buffered client completion and disposal before closing the HTTP/2 loopback connection. Cancel client work before server cleanup on failure, without changing shared teardown defaults or suppressing native errors. 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 |
ManickaP
approved these changes
Sep 15, 2026
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues were identified, and the targeted change was validated.
Pull request overview
Updates the Latin1 header test to prevent premature WinHTTP HTTP/2 connection teardown.
Changes:
- Coordinates client completion with server shutdown.
- Adds cancellation and explicit HTTP/2 cleanup for WinHTTP.
- Preserves existing protocol paths and header assertions.
File summaries
| File | Description |
|---|---|
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs |
Adds coordinated WinHTTP HTTP/2 client/server lifetime handling. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
This was referenced Sep 15, 2026
Member
Author
|
I will hold on backporting for a bit to first see if the test failure counts drop on main |
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 18, 2026
) ## Summary Keep the HTTP/2 loopback connection alive until the Latin1 header test's buffered `HttpClient.SendAsync` and client disposal finish. The previous helper starts server-side send shutdown immediately after writing the final response, potentially racing pending WinHTTP native I/O. The new path is limited to **WinHTTP HTTP/2 in this one header-encoding theory**. It preserves request-body consumption, GOAWAY-before-response ordering, the empty 200/END_STREAM response, and all raw Latin1/Huffman header assertions. Client completion is signaled in `finally`; server failure cancels the request **before** asynchronous connection disposal. HTTP/1 and Sockets retain the existing server helper. There is no production change, retry, native-error suppression, timeout increase, test disabling, or change to global loopback teardown defaults. ## Evidence and limits The supplied [CoreCLR build 1594333](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1594333), revision `6ed2ba318dcaa936da60f7d6bc9eb30cad54fc0c`, failed the Content/0x7f case with native 12030 during response buffering: [same-run console](https://helixr18s23ayyejvk1x8qcc.blob.core.windows.net/dotnet-runtime-refs-heads-main-a2037c901dc94f1d88/System.Net.Http.WinHttpHandler.Functional.Tests/1/console.55c334a6.log?helixlogtype=result). - Directly invoking the **unchanged archived CI test method** reproduced `API_QUERY_DATA_AVAILABLE / WINHTTP_CONNECTION_ERROR(12030)` at call 686. - A controlled reconstruction using those archived assemblies compared immediate shutdown with waiting for buffered client completion: **1 native 12030 in 10,000 ungated calls; 0 in 10,000 gated calls**. That controlled failure was at **API_WRITE_DATA**, not API_QUERY_DATA_AVAILABLE. Observer timestamps show server END_STREAM and send shutdown before the request-error callback. - The different native API phases and sparse failure count matter: this supports removing the test's unnecessary teardown race, but does **not** prove the internal native cause of the original CI failure or elimination of all WinHTTP 12030s. A separate bounded header-callback-delay experiment passed both arms and was inconclusive. This is a test-lifetime correction for header encoding, not a change to how WinHTTP handles arbitrary server disconnects. Cookie/client-certificate 12030s and badssl 12152/12002 diagnostics remain separate. ## Local validation Windows 11 build 26100, x64 CoreCLR Release with locally built Debug libraries/tests; actual runtime 12.0.0, test TFM net11.0-windows. Reused the successful clr+libs baseline through a subst path. | Final-source validation | Result | |---|---| | WinHTTP full functional suite, CoreCLR | 1,631 passed, 41 existing skips, zero failures | | WinHTTP full functional suite, .NET Framework net481 | 639 passed, 28 existing skips, zero failures | | Actual fixed Content/0x7f method, filtered native tracing enabled | 10,000/10,000 passed | | Shared Sockets Latin1 theory, unmodified HEAD and final source | Both 70 passed / same two HTTP/1 host-software-aborted failures | The Sockets async failure is tracked by dotnet#132004; its sync counterpart had the same signature. Both were reproduced with the unmodified HEAD test compiled via an artifact-only source override. Neither failure enters the new WinHTTP HTTP/2 branch, and neither was suppressed. Artifact-only failure injection into the retained method also exercised success, client assertion failure, server failure before/after the response, cancellation with an established connection, client timeout, and completion-gate timeout. All seven preserved their expected outcomes and completed both cleanup paths, including awaited server disposal, within 0.7 seconds. The final uninstrumented source was then explicitly rebuilt and the full suites and 10,000-call replay run again. Final WinHTTP build: zero warnings/errors. No x86, NativeAOT, older-Windows, or non-Windows execution is claimed. Framework coverage compiles the shared change but does not execute the HTTP/2 gate. Existing graceful-disposal/factory timeout limitations are not globally hardened by this patch. Resolves dotnet#130458 > [!NOTE] > This change and description were developed with GitHub Copilot; validation results were collected locally. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Summary
Keep the HTTP/2 loopback connection alive until the Latin1 header test's buffered
HttpClient.SendAsyncand client disposal finish. The previous helper starts server-side send shutdown immediately after writing the final response, potentially racing pending WinHTTP native I/O.The new path is limited to WinHTTP HTTP/2 in this one header-encoding theory. It preserves request-body consumption, GOAWAY-before-response ordering, the empty 200/END_STREAM response, and all raw Latin1/Huffman header assertions. Client completion is signaled in
finally; server failure cancels the request before asynchronous connection disposal. HTTP/1 and Sockets retain the existing server helper.There is no production change, retry, native-error suppression, timeout increase, test disabling, or change to global loopback teardown defaults.
Evidence and limits
The supplied CoreCLR build 1594333, revision
6ed2ba318dcaa936da60f7d6bc9eb30cad54fc0c, failed the Content/0x7f case with native 12030 during response buffering: same-run console.API_QUERY_DATA_AVAILABLE / WINHTTP_CONNECTION_ERROR(12030)at call 686.This is a test-lifetime correction for header encoding, not a change to how WinHTTP handles arbitrary server disconnects. Cookie/client-certificate 12030s and badssl 12152/12002 diagnostics remain separate.
Local validation
Windows 11 build 26100, x64 CoreCLR Release with locally built Debug libraries/tests; actual runtime 12.0.0, test TFM net11.0-windows. Reused the successful clr+libs baseline through a subst path.
The Sockets async failure is tracked by #132004; its sync counterpart had the same signature. Both were reproduced with the unmodified HEAD test compiled via an artifact-only source override. Neither failure enters the new WinHTTP HTTP/2 branch, and neither was suppressed.
Artifact-only failure injection into the retained method also exercised success, client assertion failure, server failure before/after the response, cancellation with an established connection, client timeout, and completion-gate timeout. All seven preserved their expected outcomes and completed both cleanup paths, including awaited server disposal, within 0.7 seconds. The final uninstrumented source was then explicitly rebuilt and the full suites and 10,000-call replay run again. Final WinHTTP build: zero warnings/errors.
No x86, NativeAOT, older-Windows, or non-Windows execution is claimed. Framework coverage compiles the shared change but does not execute the HTTP/2 gate. Existing graceful-disposal/factory timeout limitations are not globally hardened by this patch.
Resolves #130458
Note
This change and description were developed with GitHub Copilot; validation results were collected locally.