From 32eab2c9590e20743a60dc2332b712f869697882 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 2 Feb 2024 16:49:09 +0100 Subject: [PATCH 1/9] send a connection WINDOW_UPDATE before RTT PING --- .../SocketsHttpHandler/Http2Connection.cs | 34 +++++++++++-------- .../Http2StreamWindowManager.cs | 27 +++++++++++---- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index eca6e11d4f2833..6d72df6b437f16 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -619,7 +619,7 @@ private async ValueTask ProcessHeadersFrame(FrameHeader frameHeader) if (http2Stream != null) { http2Stream.OnHeadersStart(); - _rttEstimator.OnDataOrHeadersReceived(this); + _rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: true); headersHandler = http2Stream; } else @@ -747,21 +747,16 @@ private void ProcessDataFrame(FrameHeader frameHeader) ReadOnlySpan 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 (!endStream) + { + _rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: !windowUpdateSent); + } } _incomingBuffer.Discard(frameHeader.PayloadLength); @@ -1718,7 +1713,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); @@ -1732,7 +1727,7 @@ private void ExtendWindow(int amount) if (_pendingWindowUpdate < ConnectionWindowThreshold) { if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(_pendingWindowUpdate)} {_pendingWindowUpdate} < {ConnectionWindowThreshold}."); - return; + return false; } windowUpdateSize = _pendingWindowUpdate; @@ -1740,6 +1735,15 @@ private void ExtendWindow(int amount) } LogExceptions(SendWindowUpdateAsync(0, windowUpdateSize)); + return true; + } + + private bool ForceSendConnectionWindowUpdate() + { + if (_pendingWindowUpdate == 0) return false; + LogExceptions(SendWindowUpdateAsync(0, _pendingWindowUpdate)); + _pendingWindowUpdate = 0; + return true; } public override long GetIdleTicks(long nowTicks) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs index a9314d14ab174d..01899e53e83489 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2StreamWindowManager.cs @@ -145,13 +145,18 @@ private static TimeSpan StopwatchTicksToTimeSpan(long stopwatchTicks) // 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://github.com/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(), @@ -201,7 +206,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; @@ -211,6 +216,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}"); From 0687f331e8e7a5fcf0a6bf753acfaca324d2826e Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Fri, 2 Feb 2024 17:04:47 +0100 Subject: [PATCH 2/9] adjust tests --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index fc94b54bba975f..00e96f814bcbbe 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -2444,6 +2444,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); @@ -2504,6 +2505,7 @@ public async Task PostAsyncDuplex_RequestContentException_ResetsStream() 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); From 3f42a1092592e67087e301b91a76fe45a8af2de6 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Tue, 6 Feb 2024 18:21:51 +0100 Subject: [PATCH 3/9] add test --- ...SocketsHttpHandlerTest.Http2FlowControl.cs | 43 +++++++++++++++---- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs index f28491882ac3db..a886a7fde6139a 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2FlowControl.cs @@ -133,7 +133,7 @@ static async Task RunTest() TimeSpan.FromMilliseconds(30), TimeSpan.Zero, 2 * 1024 * 1024, - null); + maxWindowForPingStopValidation: MaxWindow); Assert.True(maxCredit <= MaxWindow); } @@ -186,19 +186,34 @@ static async Task RunTest() RemoteExecutor.Invoke(RunTest, options).Dispose(); } + [OuterLoop("Runs long")] + [Fact] + public async Task LongRunningSlowServerStream_NoInvalidPingsAreSent() + { + // A scenario similar to https://github.com/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 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 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); @@ -230,13 +245,13 @@ private static async Task 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) { @@ -264,7 +279,7 @@ private static async Task 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; @@ -275,6 +290,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 { @@ -289,10 +305,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); @@ -301,6 +325,7 @@ async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken) } else if (frame is WindowUpdateFrame windowUpdateFrame) { + pingsWithoutWindowUpdate = 0; // Ignore connection window: if (windowUpdateFrame.StreamId != streamId) continue; From 6a4df922ef3bccc3dd9ad97e702d64eb96aa29b8 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Tue, 6 Feb 2024 18:43:56 +0100 Subject: [PATCH 4/9] add logging to ForceSendConnectionWindowUpdate --- .../src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index 6d72df6b437f16..d3cdcf002321f4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -1740,7 +1740,9 @@ private bool ExtendWindow(int amount) private bool ForceSendConnectionWindowUpdate() { + if (NetEventSource.Log.IsEnabled()) Trace($"{nameof(_pendingWindowUpdate)}={_pendingWindowUpdate}"); if (_pendingWindowUpdate == 0) return false; + LogExceptions(SendWindowUpdateAsync(0, _pendingWindowUpdate)); _pendingWindowUpdate = 0; return true; From cfeb1e85dcc93144a0d3d8dfdbcf99c078ad0665 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Tue, 13 Feb 2024 16:27:03 +0100 Subject: [PATCH 5/9] http2Stream must not be null for OnDataOrHeadersReceived() --- .../src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs index d3cdcf002321f4..1aaf44bc7b1a39 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs @@ -753,7 +753,7 @@ private void ProcessDataFrame(FrameHeader frameHeader) if (frameData.Length > 0) { bool windowUpdateSent = ExtendWindow(frameData.Length); - if (!endStream) + if (http2Stream is not null && !endStream) { _rttEstimator.OnDataOrHeadersReceived(this, sendWindowUpdateBeforePing: !windowUpdateSent); } From 8e11fafdabbb035fa50d0a1d826ddc8cf95cedf0 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Tue, 13 Feb 2024 16:52:36 +0100 Subject: [PATCH 6/9] fix PostAsyncDuplex_DisposeResponseBodyAfterEndReceivedButBeforeConsumed_ResetsStreamAndThrowsOnRequestStreamWriteAndResponseStreamRead... --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 00e96f814bcbbe..b594e8b776c726 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -2867,6 +2867,7 @@ public async Task PostAsyncDuplex_FinishRequestBodyAndDisposeResponseBodyAfterEn Task responseTask = client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); connection = await server.EstablishConnectionAsync(); + connection.IgnoreWindowUpdates(); // Client should have sent the request headers, and the request stream should now be available Stream requestStream = await duplexContent.WaitForStreamAsync(); From 056a51dadf87280faad777e42400208b2ea41b6c Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Tue, 13 Feb 2024 17:38:17 +0100 Subject: [PATCH 7/9] fix more outerloop tests --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index b594e8b776c726..3402f93b8da45c 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -2766,6 +2766,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); } @@ -2839,6 +2840,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); } @@ -2867,7 +2869,6 @@ public async Task PostAsyncDuplex_FinishRequestBodyAndDisposeResponseBodyAfterEn Task responseTask = client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead); connection = await server.EstablishConnectionAsync(); - connection.IgnoreWindowUpdates(); // Client should have sent the request headers, and the request stream should now be available Stream requestStream = await duplexContent.WaitForStreamAsync(); From 6e8297929da6c54f5c9a2cac863e8c94c02f7d55 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 15 Feb 2024 00:57:30 +0100 Subject: [PATCH 8/9] fix PostAsyncDuplex_ClientSendsEndStream_Success() --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 3402f93b8da45c..0a0101bb6bf13e 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -2381,6 +2381,7 @@ public async Task PostAsyncDuplex_ClientSendsEndStream_Success() // Send response headers await connection.SendResponseHeadersAsync(streamId, endStream: false); + connection.IgnoreWindowUpdates(); HttpResponseMessage response = await responseTask; Stream responseStream = await response.Content.ReadAsStreamAsync(); From 75245d3d064071989e67b631e6eaf229a2bae6f6 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 15 Feb 2024 01:00:59 +0100 Subject: [PATCH 9/9] move IgnoreWindowUpdates() making the test code more similar to main --- .../tests/FunctionalTests/HttpClientHandlerTest.Http2.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 0a0101bb6bf13e..a612ad6e1aab19 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -2381,10 +2381,11 @@ public async Task PostAsyncDuplex_ClientSendsEndStream_Success() // Send response headers await connection.SendResponseHeadersAsync(streamId, endStream: false); - connection.IgnoreWindowUpdates(); + 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);