Skip to content

[release/8.0-staging] Send connection WINDOW_UPDATE before RTT PING #113702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Mar 19, 2025

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 SocketsHttpHandler we 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

  • A functional test has been added to emulate the customer scenario.
  • The change has been manually tested against customer endpoint (i.e. GCP-hosted service behind Google Cloud L7 External Load Balancer).
  • The change has been shipped in .NET 9. There were no reported issues related to the change.

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_UPDATE frames. In theory, some servers might have problem with that. Mitigations beyond testing the change in .NET 9:

  • We have a pre-existing switch to turn off RTT measurements entirely - System.Net.SocketsHttpHandler.Http2FlowControl.DisableDynamicWindowSizing
  • grpc-go client implementation has exactly same behavior which we are proposing for many years now (although it is gRPC only, not general HTTP/2 as our)
  • We did code review of popular HTTP/2 implementations -- nginx and nghttp2. We didn't find any DoS protection mechanism that would be triggered by more WINDOW_UPDATE frames.

@Copilot Copilot AI review requested due to automatic review settings March 19, 2025 17:52
@ghost ghost added the area-System.Net.Http label Mar 19, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@karelz karelz added this to the 8.0.x milestone Mar 25, 2025
Copy link
Member

@karelz karelz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - we should fix it, as we promised to Google a year ago.

@antonfirsov antonfirsov requested a review from a team March 25, 2025 15:35

This comment was marked as outdated.

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

Test failures are known and unrelated

@antonfirsov antonfirsov added the Servicing-consider Issue for next servicing release review label Mar 26, 2025
@antonfirsov
Copy link
Member Author

Approved over email.

@antonfirsov antonfirsov added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 28, 2025
@antonfirsov
Copy link
Member Author

/ba-g CI failures are unrelated, see #113702 (comment) for the list of HTTP failures

@antonfirsov antonfirsov merged commit 281540d into dotnet:release/8.0-staging Mar 28, 2025
115 of 124 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants