Correct HTTP/2 stream window cap test expectations - #133942
Merged
Merged
Conversation
Remove the stale expectation that connection RTT PINGs stop when a stream reaches its cap. Preserve flow-control safety checks and add exact WINDOW_UPDATE coverage for clamping and replenishment at the maximum. 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.
🟢 Approval recommended
The test-only changes are focused and fully validated.
Pull request overview
Updates HTTP/2 flow-control tests without changing product behavior.
Changes:
- Removes the obsolete RTT PING expectation.
- Adds exact stream-window cap and replenishment coverage.
- Preserves existing payload, frame, and PING-flood checks.
File summaries
| File | Description |
|---|---|
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs |
Updates flow-control expectations and adds exact-cap tests. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
ManickaP
approved these changes
Sep 15, 2026
This was referenced Sep 15, 2026
Member
Author
|
/ba-g Build failure is unrelated |
Member
Author
|
/backport to release/10.0 |
Contributor
|
Started backporting to |
4 tasks
Member
Author
|
/backport to release/11.0 |
Contributor
|
Started backporting to |
jtschuster
pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 18, 2026
Fixes failure in System.Net.Http.Functional.Tests.SocketsHttpHandler_Http2FlowControl_Test.MaxStreamWindowSize_WhenSet_WindowDoesNotScaleAboveMaximum observed e.g. in https://helixr18s23ayyejvk1x8qcc.blob.core.windows.net/dotnet-runtime-refs-heads-main-599d096511a54659b1/System.Net.Http.Functional.Tests/1/console.a3d71edc.log?helixlogtype=result (no tracking issue yet) ## Summary Remove the stale expectation in `TestClientWindowScalingAsync` that RTT PINGs stop once observed stream credit exceeds 90% of its maximum. RTT estimation belongs to the connection, not an individual stream. The original implementation explicitly removed this optimization because it ignored other streams ([author explanation](dotnet#54755 (comment))). No product PING behavior changes are included. The current assertion fails in the remote child before `maxCredit <= MaxWindow` is evaluated; examples include [Windows](https://helixr18s23ayyejvk1x8qcc.blob.core.windows.net/dotnet-runtime-refs-heads-main-599d096511a54659b1/System.Net.Http.Functional.Tests/1/console.a3d71edc.log?helixlogtype=result) and [macOS](https://helixr1107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-main-be1a20d5b78e45c39c/System.Net.Http.Functional.Tests/1/console.7bbcc8f9.log?helixlogtype=result) in main build 1590723. These failures do not establish a receive-window overflow. Retain the existing maximum-credit, payload-length, unexpected-frame and PING-flood checks. Add a focused theory that sends exactly one update threshold of nonfinal DATA, stops sending, then checks the target stream's WINDOW_UPDATE restores credit to exactly 654321. Initial windows 327161 and 654321 cover one-byte-over-cap doubling and replenishment at the cap. A zero scaling multiplier removes dependence on bandwidth; the complete response bytes are also verified. ## Validation Windows x64, CoreCLR Release and libraries/tests Debug, using a short `subst` path: - `build.cmd clr+libs -rc release`: succeeded, zero warnings/errors. - Full `SocketsHttpHandler_Http2FlowControl_Test` class with `/p:Outerloop=true`: **11 passed, 0 failed, 0 skipped**, after restoring all diagnostic changes. - New exact-credit theory repeated ten times: **20 passed, 0 failed, 0 skipped**. - Controlled stale-assertion reproduction: temporarily start the original helper at the cap. The old heuristic fails with the CI child `Assert.Null` signature; the corrected helper passes the identical setup. This diagnostic setup is not included in the commit. - Broken-cap negative control: temporarily remove `Math.Min` and its adjacent product `Debug.Assert` (so the assertion does not preempt the test oracle). The new growth case fails with **expected 654321, actual 654322**; the at-cap case passes. Both product changes were restored, the product rebuilt, and the full class rerun successfully. Targeted command from the repository root: ```powershell .\.dotnet\dotnet.exe build src\libraries\System.Net.Http\tests\FunctionalTests\System.Net.Http.Functional.Tests.csproj /t:Test /p:RuntimeConfiguration=Release /p:Outerloop=true '/p:XUnitOptions=-class System.Net.Http.Functional.Tests.SocketsHttpHandler_Http2FlowControl_Test' ``` The unchanged original test passed once locally; its CI failure is intermittent. macOS execution and the broader HTTP functional suite were not run locally. No CI reruns or test disabling were used. > [!NOTE] > This PR and its description were generated with GitHub Copilot. 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.
Fixes failure in System.Net.Http.Functional.Tests.SocketsHttpHandler_Http2FlowControl_Test.MaxStreamWindowSize_WhenSet_WindowDoesNotScaleAboveMaximum observed e.g. in
https://helixr18s23ayyejvk1x8qcc.blob.core.windows.net/dotnet-runtime-refs-heads-main-599d096511a54659b1/System.Net.Http.Functional.Tests/1/console.a3d71edc.log?helixlogtype=result
(no tracking issue yet)
Summary
Remove the stale expectation in
TestClientWindowScalingAsyncthat RTT PINGs stop once observed stream credit exceeds 90% of its maximum. RTT estimation belongs to the connection, not an individual stream. The original implementation explicitly removed this optimization because it ignored other streams (author explanation). No product PING behavior changes are included.The current assertion fails in the remote child before
maxCredit <= MaxWindowis evaluated; examples include Windows and macOS in main build 1590723. These failures do not establish a receive-window overflow.Retain the existing maximum-credit, payload-length, unexpected-frame and PING-flood checks. Add a focused theory that sends exactly one update threshold of nonfinal DATA, stops sending, then checks the target stream's WINDOW_UPDATE restores credit to exactly 654321. Initial windows 327161 and 654321 cover one-byte-over-cap doubling and replenishment at the cap. A zero scaling multiplier removes dependence on bandwidth; the complete response bytes are also verified.
Validation
Windows x64, CoreCLR Release and libraries/tests Debug, using a short
substpath:build.cmd clr+libs -rc release: succeeded, zero warnings/errors.SocketsHttpHandler_Http2FlowControl_Testclass with/p:Outerloop=true: 11 passed, 0 failed, 0 skipped, after restoring all diagnostic changes.Assert.Nullsignature; the corrected helper passes the identical setup. This diagnostic setup is not included in the commit.Math.Minand its adjacent productDebug.Assert(so the assertion does not preempt the test oracle). The new growth case fails with expected 654321, actual 654322; the at-cap case passes. Both product changes were restored, the product rebuilt, and the full class rerun successfully.Targeted command from the repository root:
The unchanged original test passed once locally; its CI failure is intermittent. macOS execution and the broader HTTP functional suite were not run locally. No CI reruns or test disabling were used.
Note
This PR and its description were generated with GitHub Copilot.