From 8cc3ee02363f4bc86a9470f7cdd71baf8100f901 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 May 2025 18:17:37 +0000 Subject: [PATCH 1/2] Fix HTTP/2 pings with zero connection lifetime Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../HttpConnectionPoolManager.cs | 5 +- ...cketsHttpHandlerTest.Http2KeepAlivePing.cs | 112 ++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs index 27181c7b5b8a95..8424d1b26cebe2 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs @@ -69,10 +69,13 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings) // However, we can only do such optimizations if we're not also tracking // connections per server, as we use data in the associated data structures // to do that tracking. + // Additionally, we should not avoid storing connections if keep-alive ping is configured, + // as the heartbeat timer is needed for ping functionality. bool avoidStoringConnections = settings._maxConnectionsPerServer == int.MaxValue && (settings._pooledConnectionIdleTimeout == TimeSpan.Zero || - settings._pooledConnectionLifetime == TimeSpan.Zero); + settings._pooledConnectionLifetime == TimeSpan.Zero) && + settings._keepAlivePingDelay == Timeout.InfiniteTimeSpan; // Start out with the timer not running, since we have no pools. // When it does run, run it with a frequency based on the idle timeout. diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs index 0f1c1b8d58c326..b60211bc21e56f 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs @@ -255,6 +255,118 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => }, NoAutoPingResponseHttp2Options); } + [OuterLoop("Runs long")] + [Fact] + public async Task KeepAlivePing_ZeroConnectionLifetime_PingsStillWork() + { + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); + handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(10); + handler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests; + handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + handler.PooledConnectionLifetime = TimeSpan.Zero; // Zero lifetime should still allow pings + + using HttpClient client = new HttpClient(handler); + client.DefaultRequestVersion = HttpVersion.Version20; + + // Warmup request to create connection: + HttpResponseMessage response0 = await client.GetAsync(uri); + Assert.Equal(HttpStatusCode.OK, response0.StatusCode); + + // Actual request: + HttpResponseMessage response1 = await client.GetAsync(uri); + Assert.Equal(HttpStatusCode.OK, response1.StatusCode); + + // Let connection live until server finishes: + await _serverFinished.Task.WaitAsync(TestTimeout); + }, + async server => + { + await EstablishConnectionAsync(server); + + // Warmup the connection. + int streamId1 = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId1)); + + // Request under the test scope. + int streamId2 = await ReadRequestHeaderAsync(); + Interlocked.Exchange(ref _pingCounter, 0); // reset the PING counter + + // Simulate inactive period: + await Task.Delay(5_000); + + // Even with zero lifetime, we should still receive pings for active streams: + // We may receive one RTT PING in response to HEADERS. + // Upon that, we expect to receive at least 1 keep alive PING: + Assert.True(_pingCounter > 1); + + // Finish the response: + await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId2)); + + await TerminateLoopbackConnectionAsync(); + + List unexpectedFrames = new List(); + while (_framesChannel.Reader.Count > 0) + { + Frame unexpectedFrame = await _framesChannel.Reader.ReadAsync(); + unexpectedFrames.Add(unexpectedFrame); + } + + Assert.False(unexpectedFrames.Any(), "Received unexpected frames: \n" + string.Join('\n', unexpectedFrames.Select(f => f.ToString()).ToArray())); + }, NoAutoPingResponseHttp2Options); + } + + [OuterLoop("Runs long")] + [Fact] + public async Task KeepAlivePing_ZeroConnectionLifetime_NoPingConfig_NoPingsSent() + { + await Http2LoopbackServer.CreateClientAndServerAsync(async uri => + { + SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); + // Don't configure KeepAlivePing settings + handler.PooledConnectionLifetime = TimeSpan.Zero; // Zero lifetime + + using HttpClient client = new HttpClient(handler); + client.DefaultRequestVersion = HttpVersion.Version20; + + // Warmup request to create connection: + HttpResponseMessage response0 = await client.GetAsync(uri); + Assert.Equal(HttpStatusCode.OK, response0.StatusCode); + + // Actual request: + HttpResponseMessage response1 = await client.GetAsync(uri); + Assert.Equal(HttpStatusCode.OK, response1.StatusCode); + + // Let connection live until server finishes: + await _serverFinished.Task.WaitAsync(TestTimeout); + }, + async server => + { + await EstablishConnectionAsync(server); + + // Warmup the connection. + int streamId1 = await ReadRequestHeaderAsync(); + await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId1)); + + // Request under the test scope. + int streamId2 = await ReadRequestHeaderAsync(); + Interlocked.Exchange(ref _pingCounter, 0); // reset the PING counter + + // Simulate inactive period: + await Task.Delay(5_000); + + // With zero lifetime and no ping config, we should not receive keep alive pings + // We may have received one RTT PING in response to HEADERS, but should receive no KeepAlive PING + Assert.True(_pingCounter <= 1); + + // Finish the response: + await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId2)); + + await TerminateLoopbackConnectionAsync(); + }, NoAutoPingResponseHttp2Options); + } + private async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken) { try From dbfa998a239c61c6496f61bad843683891354924 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 17 Jun 2025 16:13:54 -0400 Subject: [PATCH 2/2] Extend existing test instead of adding new ones --- ...cketsHttpHandlerTest.Http2KeepAlivePing.cs | 120 +----------------- 1 file changed, 5 insertions(+), 115 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs index b60211bc21e56f..e2af99c6de45fa 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs @@ -95,9 +95,10 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => [OuterLoop("Runs long")] [Theory] - [InlineData(HttpKeepAlivePingPolicy.Always)] - [InlineData(HttpKeepAlivePingPolicy.WithActiveRequests)] - public async Task KeepAliveConfigured_KeepAlivePingsAreSentAccordingToPolicy(HttpKeepAlivePingPolicy policy) + [InlineData(HttpKeepAlivePingPolicy.Always, -1)] + [InlineData(HttpKeepAlivePingPolicy.WithActiveRequests, -1)] + [InlineData(HttpKeepAlivePingPolicy.WithActiveRequests, 0)] + public async Task KeepAliveConfigured_KeepAlivePingsAreSentAccordingToPolicy(HttpKeepAlivePingPolicy policy, int connectionLifetimeMilliseconds) { await Http2LoopbackServer.CreateClientAndServerAsync(async uri => { @@ -105,6 +106,7 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(10); handler.KeepAlivePingPolicy = policy; handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); + handler.PooledConnectionLifetime = TimeSpan.FromMilliseconds(connectionLifetimeMilliseconds); using HttpClient client = new HttpClient(handler); client.DefaultRequestVersion = HttpVersion.Version20; @@ -255,118 +257,6 @@ await Http2LoopbackServer.CreateClientAndServerAsync(async uri => }, NoAutoPingResponseHttp2Options); } - [OuterLoop("Runs long")] - [Fact] - public async Task KeepAlivePing_ZeroConnectionLifetime_PingsStillWork() - { - await Http2LoopbackServer.CreateClientAndServerAsync(async uri => - { - SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); - handler.KeepAlivePingTimeout = TimeSpan.FromSeconds(10); - handler.KeepAlivePingPolicy = HttpKeepAlivePingPolicy.WithActiveRequests; - handler.KeepAlivePingDelay = TimeSpan.FromSeconds(1); - handler.PooledConnectionLifetime = TimeSpan.Zero; // Zero lifetime should still allow pings - - using HttpClient client = new HttpClient(handler); - client.DefaultRequestVersion = HttpVersion.Version20; - - // Warmup request to create connection: - HttpResponseMessage response0 = await client.GetAsync(uri); - Assert.Equal(HttpStatusCode.OK, response0.StatusCode); - - // Actual request: - HttpResponseMessage response1 = await client.GetAsync(uri); - Assert.Equal(HttpStatusCode.OK, response1.StatusCode); - - // Let connection live until server finishes: - await _serverFinished.Task.WaitAsync(TestTimeout); - }, - async server => - { - await EstablishConnectionAsync(server); - - // Warmup the connection. - int streamId1 = await ReadRequestHeaderAsync(); - await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId1)); - - // Request under the test scope. - int streamId2 = await ReadRequestHeaderAsync(); - Interlocked.Exchange(ref _pingCounter, 0); // reset the PING counter - - // Simulate inactive period: - await Task.Delay(5_000); - - // Even with zero lifetime, we should still receive pings for active streams: - // We may receive one RTT PING in response to HEADERS. - // Upon that, we expect to receive at least 1 keep alive PING: - Assert.True(_pingCounter > 1); - - // Finish the response: - await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId2)); - - await TerminateLoopbackConnectionAsync(); - - List unexpectedFrames = new List(); - while (_framesChannel.Reader.Count > 0) - { - Frame unexpectedFrame = await _framesChannel.Reader.ReadAsync(); - unexpectedFrames.Add(unexpectedFrame); - } - - Assert.False(unexpectedFrames.Any(), "Received unexpected frames: \n" + string.Join('\n', unexpectedFrames.Select(f => f.ToString()).ToArray())); - }, NoAutoPingResponseHttp2Options); - } - - [OuterLoop("Runs long")] - [Fact] - public async Task KeepAlivePing_ZeroConnectionLifetime_NoPingConfig_NoPingsSent() - { - await Http2LoopbackServer.CreateClientAndServerAsync(async uri => - { - SocketsHttpHandler handler = CreateSocketsHttpHandler(allowAllCertificates: true); - // Don't configure KeepAlivePing settings - handler.PooledConnectionLifetime = TimeSpan.Zero; // Zero lifetime - - using HttpClient client = new HttpClient(handler); - client.DefaultRequestVersion = HttpVersion.Version20; - - // Warmup request to create connection: - HttpResponseMessage response0 = await client.GetAsync(uri); - Assert.Equal(HttpStatusCode.OK, response0.StatusCode); - - // Actual request: - HttpResponseMessage response1 = await client.GetAsync(uri); - Assert.Equal(HttpStatusCode.OK, response1.StatusCode); - - // Let connection live until server finishes: - await _serverFinished.Task.WaitAsync(TestTimeout); - }, - async server => - { - await EstablishConnectionAsync(server); - - // Warmup the connection. - int streamId1 = await ReadRequestHeaderAsync(); - await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId1)); - - // Request under the test scope. - int streamId2 = await ReadRequestHeaderAsync(); - Interlocked.Exchange(ref _pingCounter, 0); // reset the PING counter - - // Simulate inactive period: - await Task.Delay(5_000); - - // With zero lifetime and no ping config, we should not receive keep alive pings - // We may have received one RTT PING in response to HEADERS, but should receive no KeepAlive PING - Assert.True(_pingCounter <= 1); - - // Finish the response: - await GuardConnectionWriteAsync(() => _connection.SendDefaultResponseAsync(streamId2)); - - await TerminateLoopbackConnectionAsync(); - }, NoAutoPingResponseHttp2Options); - } - private async Task ProcessIncomingFramesAsync(CancellationToken cancellationToken) { try