-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix HTTP/2 pings not working with zero connection lifetime #115735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging subscribers to this area: @dotnet/ncl |
The new test file will need to be added to the .csproj. Otherwise, it won't be included in the built tests. |
I've added the test file to the System.Net.Http.Functional.Tests.csproj file so it will be included in the built tests (f1088d8). |
Please write the test(s) in the same style as the rest of the tests and include it or then in an existing test file rather than creating a new one. |
I've moved the test methods into the existing SocketsHttpHandlerTest.Http2KeepAlivePing.cs file and removed the separate test file as requested. The tests follow the same style and patterns as the existing tests in that file (2fc0eb7). Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: stephentoub <[email protected]>
2fc0eb7
to
dbfa998
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures HTTP/2 keep-alive pings continue working when PooledConnectionLifetime
is zero by adjusting the connection‐pool optimization and adding tests.
- Update
avoidStoringConnections
to skip the optimization only when no pings are configured. - Enhance functional tests to cover infinite and zero connection lifetimes with ping enabled.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs | Parameterize the test with connectionLifetimeMilliseconds and set PooledConnectionLifetime accordingly |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs | Extend the zero‐lifetime optimization to require KeepAlivePingDelay to be infinite before avoiding stored connections |
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Http2KeepAlivePing.cs:98
- Add a test case for
HttpKeepAlivePingPolicy.Always
withconnectionLifetimeMilliseconds = 0
(e.g.InlineData(HttpKeepAlivePingPolicy.Always, 0)
) to verify pings are sent when lifetime is zero under the Always policy.
[InlineData(HttpKeepAlivePingPolicy.Always, -1)]
/ba-g infra failures |
Issue
When
PooledConnectionLifetime
is set toTimeSpan.Zero
, HTTP/2 keep-alive pings don't work, even if explicitly configured. This is because the connection pool manager optimization that avoids storing connections (when lifetime is zero) skips creating the heartbeat timer needed for ping functionality.Root Cause
In
HttpConnectionPoolManager.cs
, there's this optimization condition:When this condition is true, the code skips creating the heartbeat timer, regardless of whether the user has configured HTTP/2 keep-alive pings.
Fix
I've modified the condition to also consider whether keep-alive pings are enabled:
This ensures that if pings are configured (
keepAlivePingDelay
is notInfiniteTimeSpan
), connections will be stored and the heartbeat timer will be created, even if the connection lifetime is zero.I've also added test cases to verify this fix:
PooledConnectionLifetime = TimeSpan.Zero
and ping settings are configuredFixes #113532.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
dc.services.visualstudio.com
/home/REDACTED/work/runtime/runtime/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/runtime/runtime/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/runtime/runtime/artifacts/toolset/10.0.0-beta.25260.104.txt
(dns block)pkgs.dev.azure.com
/home/REDACTED/work/runtime/runtime/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/runtime/runtime/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/runtime/runtime/artifacts/toolset/10.0.0-beta.25260.104.txt
(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.