[release/10.0] Correct HTTP/2 stream window cap test expectations - #134035
Closed
github-actions[bot] wants to merge 1 commit into
Closed
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
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](#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>
|
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 |
Member
|
Typo, shouldve targeted 11.0 |
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
1 participant
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.
Backport of #133942 to release/10.0
/cc @rzikm
Customer Impact
[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]
Regression
[If yes, specify when the regression was introduced. Provide the PR or commit if known.]
Testing
[How was the fix verified? How was the issue missed previously? What tests were added?]
Risk
[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging, notrelease/X.0.release/X.0(no-stagingsuffix).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.