Send connection WINDOW_UPDATE before RTT PING #97881

antonfirsov · 2024-02-02T16:16:15Z

Some servers in GCP have a non-standard ping accounting mechanism, meaning they reset their unsolicited PING counter when they receive DATA, HEADERS or WINDOW_UPDATE. To comply with this behavior, this PR adjusts the RTT logic ensuring that a connection WINDOW_UPDATE is being sent out before we send an RTT PING by applying two changes:

Swap the order of the normal connection WINDOW_UPDATE we attempt after every DATA frame received and the sending of the RTT PING so WINDOW_UPDATE goes first.
In case we need to send an RTT PING, and we didn't send a normal connection WINDOW_UPDATE (threshold not reached), send a WINDOW_UPDATE anyways with current _pendingWindowUpdate credits. Do not send a PING if this is not possible (_pendingWindowUpdate == 0). Just like RTT PINGs, such WINDOW_UPDATEs are relatively rare and should not hurt performance.

Ideally, this fix should be backported to all supported versions.

ghost · 2024-02-02T16:16:28Z

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

Issue Details

Fixes #97131.

Some servers in GCP have a non-standard ping accounting mechanism, meaning they reset their unsolicited PING counter when receiving DATA, HEADERS or WINDOW_UPDATE. To comply with this behavior, this PR adjusts the RTT logic ensuring that a connection WINDOW_UPDATE is being sent out before we send an RTT PING by doing two changes:

Swap the order of the normal connection WINDOW_UPDATE we attempt after every DATA frame received and the sending of the RTT PING
In case we need to send an RTT PING, and we didn't send a normal connection WINDOW_UPDATE (because of reaching the threshold), send a WINDOW_UPDATE anyways by sending out all the _pendingWindowUpdate credits. Do not send a PING if this is not possible (_pendingWindowUpdate == 0).

Ideally, this fix should be backported to all supported versions.

Author:	antonfirsov
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

antonfirsov · 2024-02-02T16:18:21Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

+            if (_pendingWindowUpdate == 0) return false;
+            LogExceptions(SendWindowUpdateAsync(0, _pendingWindowUpdate));
+            _pendingWindowUpdate = 0;
+            return true;


ExtendWindow has unnecessary synchronization around _pendingWindowUpdate manipulation which is didn't add to this method. See #97878.

JamesNK

This is lite on unit tests. What about tests for scenarios that will and won't trigger a ping? And a test that fails if WINDOWS_UPDATE isn't present.

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs

antonfirsov · 2024-02-06T17:45:05Z

Pushed a test in e93920a.

wfurt

LGTM.
I don't fully understand the window & ping management but I don't have any concerns. Hopefully testing was done to ensure correctness.
And I guess excessive updates is the biggest risk of this change, right?

antonfirsov · 2024-02-08T21:53:30Z

excessive updates is the biggest risk of this change, right?

The amount of WINDOW_UPDATE-s we send did not get significantly higher. Now we essentially do the same as the grpc-go client. Their code has been around for quite a while, and I'm not aware of any servers having an issue with this logic.

https://github.com/grpc/grpc-go/blob/d41b01db97ca2e3627b2c9949fffe8f152a4255d/internal/transport/http2_client.go#L1128-L1140

ManickaP

LGTM

MihaZupan

LGTM.

Side question: as far as I understand our current logic, we'll keep sending pings once every ~2 seconds, but only update the RTT if it gets lower.
Are the updates after the initial burst meaningful? That is, we expect the RTT might decrease, but we don't care if it increases?

antonfirsov · 2024-02-10T14:02:51Z

[Deleted my previous comment because it did not make sense.]

Adjusting RTT upwards would risk growing the window "accidentally" in case of an occasional high delay measurement:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs

Lines 108 to 109 in 4928f3c

    
           // The condition to extend the window is: 
        
           // (_deliveredBytes / dt) * rtt > _streamWindowSize * _windowScaleThresholdMultiplier

With an unstable connection the minimum delay is hopefully constant in long run for most practical SocketsHttpHandler use cases, or at least this is the assumption behind the current strategy.

…med_ResetsStreamAndThrowsOnRequestStreamWriteAndResponseStreamRead...

antonfirsov · 2024-02-13T16:40:02Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2024-02-13T16:40:11Z

Azure Pipelines successfully started running 1 pipeline(s).

antonfirsov · 2024-02-13T20:56:03Z

CI failures are unrelated, the massive outerloop failures are caused by #96035.

antonfirsov · 2024-02-13T20:57:44Z

/backport to release/8.0-staging

github-actions · 2024-02-13T20:57:56Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7892640557

antonfirsov · 2024-02-13T20:58:00Z

/backport to release/7.0-staging

github-actions · 2024-02-13T20:58:11Z

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/7892642858

antonfirsov · 2024-02-13T20:58:22Z

/backport to release/6.0-staging

github-actions · 2024-02-13T20:58:34Z

Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/7892646182

antonfirsov · 2025-03-19T14:23:18Z

/backport to release/8.0-staging

github-actions · 2025-03-19T14:23:31Z

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/13948929374

github-actions · 2025-03-19T14:25:15Z

@antonfirsov backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: send a connection WINDOW_UPDATE before RTT PING
Using index info to reconstruct a base tree...
M	src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
CONFLICT (content): Merge conflict in src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 send a connection WINDOW_UPDATE before RTT PING
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

antonfirsov added 2 commits February 2, 2024 16:49

send a connection WINDOW_UPDATE before RTT PING

6af5b27

adjust tests

63c5ebd

ghost added the area-System.Net.Http label Feb 2, 2024

ghost assigned antonfirsov Feb 2, 2024

antonfirsov commented Feb 2, 2024

View reviewed changes

antonfirsov requested a review from a team February 2, 2024 16:24

build-analysis bot mentioned this pull request Feb 2, 2024

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049

Closed

Merge branch 'main' into adjust-rtt-logic

7064b1f

antonfirsov mentioned this pull request Feb 5, 2024

gRPC stream "Response ended prematurely (ResponseEnded)" grpc/grpc-dotnet#2361

Open

JamesNK reviewed Feb 6, 2024

View reviewed changes

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs Show resolved Hide resolved

...libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs Show resolved Hide resolved

antonfirsov added 3 commits February 6, 2024 16:28

Merge branch 'main' into adjust-rtt-logic

2701197

add test

e93920a

add logging to ForceSendConnectionWindowUpdate

82214a9

Merge branch 'main' into adjust-rtt-logic

4928f3c

wfurt approved these changes Feb 8, 2024

View reviewed changes

ManickaP approved these changes Feb 9, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

build-analysis bot mentioned this pull request Feb 9, 2024

XUnitLogChecker: runtime-libraries-coreclr outerloop tests are unable to find the tool DLL #96035

Closed

MihaZupan approved these changes Feb 9, 2024

View reviewed changes

antonfirsov added 4 commits February 13, 2024 16:27

http2Stream must not be null for OnDataOrHeadersReceived()

ccdcdb3

fix PostAsyncDuplex_DisposeResponseBodyAfterEndReceivedButBeforeConsu…

c75d736

…med_ResetsStreamAndThrowsOnRequestStreamWriteAndResponseStreamRead...

Merge branch 'main' into adjust-rtt-logic

b3ec201

fix more outerloop tests

e82fee2

antonfirsov merged commit fae6720 into dotnet:main Feb 13, 2024

This was referenced Feb 13, 2024

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

Closed

[release/7.0-staging] Send connection WINDOW_UPDATE before RTT PING #98384

Closed

[release/6.0-staging] Send connection WINDOW_UPDATE before RTT PING #98385

Closed

build-analysis bot mentioned this pull request Feb 14, 2024

[browser][MT] Assert failed: Cannot find Promise for JSHandle -2 #98406

Closed

github-actions bot locked and limited conversation to collaborators Mar 16, 2024

karelz added this to the 9.0.0 milestone May 14, 2024

github-actions bot unlocked this conversation Mar 19, 2025

github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2025

Send connection WINDOW_UPDATE before RTT PING #97881

Send connection WINDOW_UPDATE before RTT PING #97881

Uh oh!

Conversation

antonfirsov commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 2, 2024

Uh oh!

antonfirsov Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antonfirsov commented Feb 6, 2024

Uh oh!

wfurt left a comment

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Feb 8, 2024

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

MihaZupan left a comment

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonfirsov commented Feb 13, 2024

Uh oh!

azure-pipelines bot commented Feb 13, 2024

Uh oh!

antonfirsov commented Feb 13, 2024

Uh oh!

antonfirsov commented Feb 13, 2024

Uh oh!

github-actions bot commented Feb 13, 2024

Uh oh!

antonfirsov commented Feb 13, 2024

Uh oh!

github-actions bot commented Feb 13, 2024

Uh oh!

antonfirsov commented Feb 13, 2024

Uh oh!

github-actions bot commented Feb 13, 2024

Uh oh!

antonfirsov commented Mar 19, 2025

Uh oh!

github-actions bot commented Mar 19, 2025

Uh oh!

github-actions bot commented Mar 19, 2025

Uh oh!

Uh oh!

antonfirsov commented Feb 2, 2024 •

edited

Loading

antonfirsov Feb 2, 2024 •

edited

Loading

antonfirsov commented Feb 10, 2024 •

edited

Loading