Send connection WINDOW_UPDATE before RTT PING

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

@@ -637,7 +637,7 @@ private async ValueTask ProcessHeadersFrame(FrameHeader frameHeader)
  if (http2Stream != null)
  {
  http2Stream.OnHeadersStart();
- _rttEstimator.OnDataOrHeadersReceived(this);
+ _rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: true);
  headersHandler = http2Stream;
  }
  else
@@ -766,21 +766,16 @@ private void ProcessDataFrame(FrameHeader frameHeader)
 
  ReadOnlySpan<byte> frameData = GetFrameData(_incomingBuffer.ActiveSpan.Slice(0, frameHeader.PayloadLength), hasPad: frameHeader.PaddedFlag, hasPriority: false);
 
- if (http2Stream != null)
- {
- bool endStream = frameHeader.EndStreamFlag;
-
- http2Stream.OnResponseData(frameData, endStream);
-
- if (!endStream && frameData.Length > 0)
- {
- _rttEstimator.OnDataOrHeadersReceived(this);
- }
- }
+ bool endStream = frameHeader.EndStreamFlag;
+ http2Stream?.OnResponseData(frameData, endStream);
 
  if (frameData.Length > 0)
  {
- ExtendWindow(frameData.Length);
+ bool windowUpdateSent = ExtendWindow(frameData.Length);
+ if (http2Stream is not null && !endStream)
+ {
+ _rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: !windowUpdateSent);
+ }
  }
 
  _incomingBuffer.Discard(frameHeader.PayloadLength);
@@ -1772,7 +1767,7 @@ private Task SendWindowUpdateAsync(int streamId, int amount)
  });
  }
 
- private void ExtendWindow(int amount)
+ private bool ExtendWindow(int amount)
  {
  if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(amount)}={amount}");
  Debug.Assert(amount > 0);
@@ -1786,14 +1781,25 @@ private void ExtendWindow(int amount)
  if (_pendingWindowUpdate < ConnectionWindowThreshold)
  {
  if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(_pendingWindowUpdate)} {_pendingWindowUpdate} < {ConnectionWindowThreshold}.");
- return;
+ return false;
  }
 
  windowUpdateSize = _pendingWindowUpdate;
  _pendingWindowUpdate = 0;
  }
 
  LogExceptions(SendWindowUpdateAsync(0, windowUpdateSize));
+ return true;
+ }
+
+ private bool ForceSendConnectionWindowUpdate()
+ {
+ if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(_pendingWindowUpdate)}={_pendingWindowUpdate}");
+ if (_pendingWindowUpdate == 0) return false;
+
+ LogExceptions(SendWindowUpdateAsync(0, _pendingWindowUpdate));
+ _pendingWindowUpdate = 0;
+ return true;
  }
 
  public override long GetIdleTicks(long nowTicks)

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

@@ -138,13 +138,18 @@ private void AdjustWindowDynamic(int bytesConsumed, Http2Stream stream)
  // Assuming that the network characteristics of the connection wouldn't change much within its lifetime, we are maintaining a running minimum value.
  // The more PINGs we send, the more accurate is the estimation of MinRtt, however we should be careful not to send too many of them,
  // to avoid triggering the server's PING flood protection which may result in an unexpected GOAWAY.
- // With most servers we are fine to send PINGs, as long as we are reading their data, this rule is well formalized for gRPC:
+ //
+ // Several strategies have been implemented to conform with real life servers.
+ // 1. With most servers we are fine to send PINGs as long as we are reading their data, a rule formalized by a gRPC spec:
  // https:/grpc/proposal/blob/master/A8-client-side-keepalive.md
- // As a rule of thumb, we can send send a PING whenever we receive DATA or HEADERS, however, there are some servers which allow receiving only
- // a limited amount of PINGs within a given timeframe.
- // To deal with the conflicting requirements:
- // - We send an initial burst of 'InitialBurstCount' PINGs, to get a relatively good estimation fast
- // - Afterwards, we send PINGs with the maximum frequency of 'PingIntervalInSeconds' PINGs per second
+ // According to this rule, we are OK to send a PING whenever we receive DATA or HEADERS, since the servers conforming to this doc
+ // will reset their unsolicited ping counter whenever they *send* DATA or HEADERS.
+ // 2. Some servers allow receiving only a limited amount of PINGs within a given timeframe.
+ // To deal with this, we send an initial burst of 'InitialBurstCount' (=4) PINGs, to get a relatively good estimation fast. Afterwards,
+ // we send PINGs each 'PingIntervalInSeconds' second, to maintain our estimation without triggering these servers.
+ // 3. Some servers in Google's backends reset their unsolicited ping counter when they *receive* DATA, HEADERS, or WINDOW_UPDATE.
+ // To deal with this, we need to make sure to send a connection WINDOW_UPDATE before sending a PING. The initial burst is an exception
+ // to this rule, since the mentioned server can tolerate 4 PINGs without receiving a WINDOW_UPDATE.
  //
  // Threading:
  // OnInitialSettingsSent() is called during initialization, all other methods are triggered by HttpConnection.ProcessIncomingFramesAsync(),
@@ -194,7 +199,7 @@ internal void OnInitialSettingsAckReceived(Http2Connection connection)
  _state = State.Waiting;
  }
 
- internal void OnDataOrHeadersReceived(Http2Connection connection)
+ internal void OnDataOrHeadersReceived(Http2Connection connection, bool sendWindowUpdateBeforePing)
  {
  if (_state != State.Waiting) return;
 
@@ -204,6 +209,14 @@ internal void OnDataOrHeadersReceived(Http2Connection connection)
  {
  if (initial) _initialBurst--;
 
+ // 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)
+ {
+ return;
+ }
+
  // Send a PING
  _pingCounter--;
  if (NetEventSource.Log.IsEnabled()) connection.Trace($"[FlowControl] Sending RTT PING with payload {_pingCounter}");

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

@@ -2453,6 +2453,7 @@ public async Task PostAsyncDuplex_ClientSendsEndStream_Success()
  HttpResponseMessage response = await responseTask;
  Stream responseStream = await response.Content.ReadAsStreamAsync();
 
+ connection.IgnoreWindowUpdates();
  // Send some data back and forth
  await SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId);
  await SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId);
@@ -2513,6 +2514,7 @@ public async Task PostAsyncDuplex_ServerSendsEndStream_Success()
  HttpResponseMessage response = await responseTask;
  Stream responseStream = await response.Content.ReadAsStreamAsync();
 
+ connection.IgnoreWindowUpdates();
  // Send some data back and forth
  await SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId);
  await SendAndReceiveResponseDataAsync(contentBytes, responseStream, connection, streamId);
@@ -2833,6 +2835,7 @@ public async Task PostAsyncDuplex_DisposeResponseBodyBeforeEnd_ResetsStreamAndTh
  // This allows the request processing to complete.
  duplexContent.Fail(e);
 
+ connection.IgnoreWindowUpdates(); // The RTT algorithm may send a WINDOW_UPDATE before RST_STREAM.
  // Client should set RST_STREAM.
  await connection.ReadRstStreamAsync(streamId);
  }
@@ -2906,6 +2909,7 @@ public async Task PostAsyncDuplex_DisposeResponseBodyAfterEndReceivedButBeforeCo
  // This allows the request processing to complete.
  duplexContent.Fail(e);
 
+ connection.IgnoreWindowUpdates(); // The RTT algorithm may send a WINDOW_UPDATE before RST_STREAM.
  // Client should set RST_STREAM.
  await connection.ReadRstStreamAsync(streamId);
  }

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs

@@ -128,7 +128,7 @@ static async Task RunTest()
  TimeSpan.FromMilliseconds(30),
  TimeSpan.Zero,
  2 * 1024 * 1024,
- null);
+ maxWindowForPingStopValidation: MaxWindow);
 
  Assert.True(maxCredit <= MaxWindow);
  }
@@ -181,19 +181,34 @@ static async Task RunTest()
  RemoteExecutor.Invoke(RunTest, options).Dispose();
  }
 
+ [OuterLoop("Runs long")]
+ [Fact]
+ public async Task LongRunningSlowServerStream_NoInvalidPingsAreSent()
+ {
+ // A scenario similar to https:/grpc/grpc-dotnet/issues/2361.
+ // We need to send a small amount of data so the connection window is not consumed and no "standard" WINDOW_UPDATEs are sent and
+ // we also need to do it very slowly to cover some RTT PINGs after the initial burst.
+ // This scenario should trigger the "forced WINDOW_UPDATE" logic in the implementation, ensuring that no more than 4 PINGs are sent without a WINDOW_UPDATE.
+ await TestClientWindowScalingAsync(
+ TimeSpan.FromMilliseconds(500),
+ TimeSpan.FromMilliseconds(500),
+ 1024,
+ _output,
+ dataPerFrame: 32);
+ }
+
  private static async Task<int> TestClientWindowScalingAsync(
  TimeSpan networkDelay,
  TimeSpan slowBandwidthSimDelay,
  int bytesToDownload,
  ITestOutputHelper output = null,
- int maxWindowForPingStopValidation = int.MaxValue, // set to actual maximum to test if we stop sending PING when window reached maximum
- Action<SocketsHttpHandler> configureHandler = null)
+ int dataPerFrame = 16384,
+ int maxWindowForPingStopValidation = 16 * 1024 * 1024) // set to actual maximum to test if we stop sending PING when window reached maximum
  {
  TimeSpan timeout = TimeSpan.FromSeconds(30);
  CancellationTokenSource timeoutCts = new CancellationTokenSource(timeout);
 
  HttpClientHandler handler = CreateHttpClientHandler(HttpVersion20.Value);
- configureHandler?.Invoke(GetUnderlyingSocketsHttpHandler(handler));
 
  using Http2LoopbackServer server = Http2LoopbackServer.CreateServer(NoAutoPingResponseHttp2Options);
  using HttpClient client = new HttpClient(handler, true);
@@ -225,13 +240,13 @@ private static async Task<int> TestClientWindowScalingAsync(
  using SemaphoreSlim writeSemaphore = new SemaphoreSlim(1);
  int remainingBytes = bytesToDownload;
 
- bool pingReceivedAfterReachingMaxWindow = false;
+ string unexpectedPingReason = null;
  bool unexpectedFrameReceived = false;
  CancellationTokenSource stopFrameProcessingCts = new CancellationTokenSource();
 
  CancellationTokenSource linkedCts = CancellationTokenSource.CreateLinkedTokenSource(stopFrameProcessingCts.Token, timeoutCts.Token);
  Task processFramesTask = ProcessIncomingFramesAsync(linkedCts.Token);
- byte[] buffer = new byte[16384];
+ byte[] buffer = new byte[dataPerFrame];
 
  while (remainingBytes > 0)
  {
@@ -259,7 +274,7 @@ private static async Task<int> TestClientWindowScalingAsync(
 
  int dataReceived = (await response.Content.ReadAsByteArrayAsync()).Length;
  Assert.Equal(bytesToDownload, dataReceived);
- Assert.False(pingReceivedAfterReachingMaxWindow, "Server received a PING after reaching max window");
+ Assert.Null(unexpectedPingReason);
  Assert.False(unexpectedFrameReceived, "Server received an unexpected frame, see test output for more details.");
 
  return maxCredit;
@@ -270,6 +285,7 @@ async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken)
  // We should not receive any more RTT PING's after this point
  int maxWindowCreditThreshold = (int) (0.9 * maxWindowForPingStopValidation);
  output?.WriteLine($"maxWindowCreditThreshold: {maxWindowCreditThreshold} maxWindowForPingStopValidation: {maxWindowForPingStopValidation}");
+ int pingsWithoutWindowUpdate = 0;
 
  try
  {
@@ -284,10 +300,18 @@ async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken)
 
  output?.WriteLine($"Received PING ({pingFrame.Data})");
 
+ pingsWithoutWindowUpdate++;
  if (maxCredit > maxWindowCreditThreshold)
  {
- output?.WriteLine("PING was unexpected");
- Volatile.Write(ref pingReceivedAfterReachingMaxWindow, true);
+ Volatile.Write(ref unexpectedPingReason, "The server received a PING after reaching max window");
+ output?.WriteLine($"PING was unexpected: {unexpectedPingReason}");
+ }
+
+ // Exceeding this limit may trigger a GOAWAY on some servers. See implementation comments for more details.
+ if (pingsWithoutWindowUpdate > 4)
+ {
+ Volatile.Write(ref unexpectedPingReason, $"The server received {pingsWithoutWindowUpdate} PINGs without receiving a WINDOW_UPDATE");
+ output?.WriteLine($"PING was unexpected: {unexpectedPingReason}");
  }
 
  await writeSemaphore.WaitAsync(cancellationToken);
@@ -296,6 +320,7 @@ async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken)
  }
  else if (frame is WindowUpdateFrame windowUpdateFrame)
  {
+ pingsWithoutWindowUpdate = 0;
  // Ignore connection window:
  if (windowUpdateFrame.StreamId != streamId) continue;