Repository navigation
Conversation
The socket pool already expires idle connections, but the HTTP client always built its pool with the default expiration (-1ULL, never) and the OSS ClientOptions had no knob for it. - Client::set_max_idle_timeout()/max_idle_timeout(), captured by each vCPU's built-in dialer on its first dial - new_http_dialer() forwards the expiration to its PoolDialer - objstore::ClientOptions::max_idle_timeout_us, passed to the HTTP client A zero expiration is now rejected as -1ULL in the TCPSocketPool constructor. Zero was harmless before only because nothing ever passed it: collect() would compute interval = min(heartbeat_interval, 0) = 0 and check_expire_heartbeat() would trip assert(near_expire.expiration() > now), or spin the collector in release builds. Clamping in the pool rather than in new_http_dialer() covers every caller of the public new_tcp_socket_pool() API, not just the HTTP path. Test: net/http/test/client_function_test.cpp max_idle_timeout_expires_pooled_connection zero_max_idle_timeout_means_no_expiration
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
The socket pool already expires idle connections, but there was no way to ask
for it: the HTTP client always built its pool with the default expiration
(
-1ULL, never), andobjstore::ClientOptionshad no knob for it either.This exposes it end to end:
Client::set_max_idle_timeout()/Client::max_idle_timeout(), captured byeach vCPU's built-in dialer on its first dial
new_http_dialer()forwards the expiration to itsPoolDialerobjstore::ClientOptions::max_idle_timeout_us, passed down to the HTTP clientDefault stays
-1ULL(never expire), so existing behaviour is unchanged.Hardening: a zero expiration is now rejected
SocketPoolArgs::expiration == 0was harmless before only because nothing everpassed it. If it reaches the pool,
collect()computesinterval = min(heartbeat_interval, 0) = 0andcheck_expire_heartbeat()tripsassert(near_expire.expiration() > now)-- or spins the collector burning avCPU in release builds, where the assert is compiled out.
Now that the value is user-reachable,
TCPSocketPool's constructor normalizes0to-1ULL. Clamping there rather than innew_http_dialer()covers everycaller of the public
new_tcp_socket_pool()API, not just the HTTP path.Tests
net/http/test/client_function_test.cpp:max_idle_timeout_expires_pooled_connectionzero_max_idle_timeout_means_no_expirationAs a negative control, removing the clamp makes the second test abort with
Assertion failed: (near_expire.expiration() > now), function check_expire_heartbeat, file net/pooled_socket.cpp.Local (macOS / kqueue, Debug):
client_function_test: 55 passed, 1 skippeddialer_test: 17 passedtest-sockpool: 32 passedNote
There is a separate, hand-written PR against
release/0.9for the same feature.It is not a cherry-pick: 0.9 has no
net/http/dialer.*, and its pool is athread_local PooledDialerinsideclient.cpp, so the plumbing and the testsetup are both different. Please do not add
need-backporthere -- theautomatic cherry-pick would conflict on
dialer.{h,cpp}(modify/delete) andcommit the conflict markers into the generated PR.