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.

JamesNK · 2024-02-06T13:46:11Z

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

+                    // When sendWindowUpdateBeforePing is true, try to send a WINDOW_UPDATE to make Google backends happy.
+                    // Unless we are doing the initial burst, do not send PING if we were not able to send the WINDOW_UPDATE.
+                    // See point 3. in the comments above the class definition for more info.
+                    if (sendWindowUpdateBeforePing && !connection.ForceSendConnectionWindowUpdate() && !initial)


Where are places that can send a windows update? Can a connection be in the situation where a window update is always being sent somewhere else and when it comes time to send a ping, the pending window update is always empty?

Where are places that can send a windows update?

For connection window the standard method is ExtendWindow the new one introduced in this PR is ForceSendConnectionWindowUpdate. There are no other places sending WINDOW_UPDATE for the connection.

Can a connection be in the situation where a window update is always being sent somewhere else, the pending window update is always empty?

ExtendWindow will send a "standard" connection WINDOW_UPDATE only if receiving DATA will make _pendingWindowUpdate pass ConnectionWindowThreshold. If that's the case, sendWindowUpdateBeforePing will be false at this line, so we won't send a WINDOW_UPDATE in DataOrHeadersReceived.

Note that receiving DATA triggers ExtendWindow and DataOrHeadersReceived sequentially (with the change in the PR WINDOW_UPDATE goes first):

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

Lines 772 to 779 in 7064b1f

if (frameData.Length > 0)

{

bool windowUpdateSent = ExtendWindow(frameData.Length);

if (!endStream)

{

_rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: !windowUpdateSent);

}

}

...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 added 2 commits February 2, 2024 16:49

send a connection WINDOW_UPDATE before RTT PING

6af5b27

adjust tests

63c5ebd

dotnet-issue-labeler bot 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

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
109 of 119 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send connection WINDOW_UPDATE before RTT PING #97881

Send connection WINDOW_UPDATE before RTT PING #97881

antonfirsov commented Feb 2, 2024 •

edited

Loading

ghost commented Feb 2, 2024

antonfirsov Feb 2, 2024 •

edited

Loading

JamesNK left a comment

JamesNK Feb 6, 2024

antonfirsov Feb 6, 2024 •

edited

Loading

antonfirsov commented Feb 6, 2024

wfurt left a comment

antonfirsov commented Feb 8, 2024

ManickaP left a comment

This comment was marked as resolved.

This comment was marked as resolved.

MihaZupan left a comment

antonfirsov commented Feb 10, 2024 •

edited

Loading

antonfirsov commented Feb 13, 2024

azure-pipelines bot commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

	if (frameData.Length > 0)
	{
	bool windowUpdateSent = ExtendWindow(frameData.Length);
	if (!endStream)
	{
	_rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: !windowUpdateSent);
	}
	}

Send connection WINDOW_UPDATE before RTT PING #97881

Send connection WINDOW_UPDATE before RTT PING #97881

Conversation

antonfirsov commented Feb 2, 2024 • edited Loading

ghost commented Feb 2, 2024

antonfirsov Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

JamesNK left a comment

Choose a reason for hiding this comment

JamesNK Feb 6, 2024

Choose a reason for hiding this comment

antonfirsov Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

antonfirsov commented Feb 6, 2024

wfurt left a comment

Choose a reason for hiding this comment

antonfirsov commented Feb 8, 2024

ManickaP left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

MihaZupan left a comment

Choose a reason for hiding this comment

antonfirsov commented Feb 10, 2024 • edited Loading

antonfirsov commented Feb 13, 2024

azure-pipelines bot commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

antonfirsov commented Feb 13, 2024

github-actions bot commented Feb 13, 2024

antonfirsov commented Feb 2, 2024 •

edited

Loading

antonfirsov Feb 2, 2024 •

edited

Loading

antonfirsov Feb 6, 2024 •

edited

Loading

antonfirsov commented Feb 10, 2024 •

edited

Loading