{{ message }}
[release/8.0-staging] Send connection WINDOW_UPDATE before RTT PING#113702
Merged
antonfirsov merged 5 commits intoMar 28, 2025
Merged
Conversation
ghost
added
the
Contributor
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request backports changes to adjust the ordering of WINDOW_UPDATE and PING frames in HTTP/2, aiming to prevent triggering unwanted DoS protections when using gRPC with Google Cloud L7 External Load Balancer. Key changes include:
- Updating comments and logic in Http2StreamWindowManager to control when a WINDOW_UPDATE is sent before a PING.
- Modifying the ExtendWindow method in Http2Connection to return a boolean indicating if a WINDOW_UPDATE was sent.
- Adjusting related tests in SocketsHttpHandlerTest and HttpClientHandlerTest to reflect these changes.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs | Updated comment and control logic for sending WINDOW_UPDATE before PING. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs | Changed the ExtendWindow method to return a boolean and updated call sites accordingly. |
| src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs | Adjusted tests to verify no unexpected PINGs are sent when the max window is reached. |
| src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | Added IgnoreWindowUpdates() calls in tests to account for new RTT algorithm behavior. |
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs:277
- Consider adding additional test cases to cover varying scenarios of connection credit thresholds to ensure that no unexpected PINGs are sent when the window update limit is reached.
Assert.Null(unexpectedPingReason);
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced Mar 19, 2025
Closed
This comment was marked as outdated.
This comment was marked as outdated.
karelz
approved these changes
Mar 25, 2025
karelz
left a comment
karelz
left a comment
Member
There was a problem hiding this comment.
LGTM - we should fix it, as we promised to Google a year ago.
MihaZupan
approved these changes
Mar 25, 2025
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
Author
Test failures are known and unrelated
|
Contributor
Author
|
Approved over email. |
antonfirsov
added
Servicing-approved
Contributor
Author
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #97881 to release/8.0-staging
Fixes #97131
Customer Impact
Problem: gRPC server-streaming connections may be closed by server when communicating with service on GCP behind Google Cloud L7 External Load Balancer. It depends on size and frequency of gRPC messages.
Reported by customers in grpc/grpc-dotnet#2361 and grpc/grpc-dotnet#2358
gRPC is built on top of HTTP/2 protocol. In
SocketsHttpHandlerwe use PING frames to measure RTT (Round-Trip Time) to leverage connections efficiently. Our usage of PING frames triggers DoS protection in Google Cloud L7 External Load Balancer and they close the connection from the server side.We worked with them to design ordering of frames mixed with WINDOW_UPDATE frames in a way that will avoid triggering their DoS protection and will allow us to measure RTT.
Regression
No. The behavior is specific to Google Cloud L7 External Load Balancer.
Testing
Risk
Low (the change has been part of .NET 9 for more than 1 year, shipping as RC/GA since summer 2024).
We are now sending higher volume of
WINDOW_UPDATEframes. In theory, some servers might have problem with that. Mitigations beyond testing the change in .NET 9:System.Net.SocketsHttpHandler.Http2FlowControl.DisableDynamicWindowSizingWINDOW_UPDATEframes.