Repository navigation
Conversation
The socket pool already supports an expiration (TTL) that was never exposed
through the API. Add Client::set_max_idle_timeout()/max_idle_timeout() and
ClientOptions::max_idle_timeout_us, defaulting to -1 (never expire).
The pool is a thread_local PooledDialer shared by all clients within a
std::thread and created on the first request, so the TTL comes from the first
client that sends a request; this is documented on new_http_client().
Two hardening fixes, since a finite TTL could never reach the pool before:
- A zero TTL made evict() compute near_expire == now, tripping
assert(near_expire > now) in debug builds and spinning the timer forever
in release builds. Zero is now rejected as -1 in PooledDialer::init().
- evict() computed near_expire with a bare TTL_us + now, which wraps around
for extreme values. Switched to sat_add().
Test: net/http/test/client_function_test.cpp
max_idle_timeout_expires_pooled_connection
zero_max_idle_timeout_means_no_expiration
lihuiba
reviewed
Nov 10, 2026
| // Create an HTTP client. Without cookie_jar, "Set-Cookies" headers are ignored. | ||
| // Note: HTTP clients within the same std::thread share TLS config and connection pool. | ||
| // Use separate std::threads for different TLS configurations. | ||
| // Note: HTTP clients within the same std::thread share TLS config and connection |
Collaborator
There was a problem hiding this comment.
use the term vcpu instead of std::thread
| std::string m_proxy_auth; | ||
| std::string m_user_agent; | ||
| uint64_t m_timeout = -1UL; | ||
| uint64_t m_max_idle_timeout = -1UL; |
Collaborator
There was a problem hiding this comment.
do NOT use UL or L (long), use ULL instead throughout Photon, for portability.
It's OK in 0.9
| // -1UL (default) keeps idle connections until the peer closes them; 0 is | ||
| // treated as -1UL. Must be set before the first request; see | ||
| // new_http_client() for the per-std::thread sharing behaviour. | ||
| void set_max_idle_timeout(uint64_t timeout_us) { m_max_idle_timeout = timeout_us; } |
Collaborator
There was a problem hiding this comment.
connection_idle_expiration is preferred. "connection" is more informative, and "expiration" is aligned with main.
Collaborator
|
PR to main is OK, and rely on auto backport for older releases. |
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 supports an expiration (TTL) that was never exposed
through the API. This adds:
Client::set_max_idle_timeout()/Client::max_idle_timeout()objstore::ClientOptions::max_idle_timeout_usDefault stays
-1(never expire), so existing behaviour is unchanged.The pool is a
thread_local PooledDialershared by all clients within astd::threadand created on the first request, so the TTL comes from the firstclient that sends a request. This is documented on
new_http_client().Hardening fixes
A finite TTL could never reach the pool before, so two latent problems are
fixed here:
evict()computenear_expire == now, trippingassert(near_expire > now)in debug builds and spinning the timer forever inrelease builds. Zero is now rejected as
-1inPooledDialer::init().evict()computednear_expirewith a bareTTL_us + now, which wrapsaround for extreme values. Switched to
sat_add().Tests
net/http/test/client_function_test.cpp:max_idle_timeout_expires_pooled_connectionzero_max_idle_timeout_means_no_expirationBoth run the client on a dedicated
std::thread, because the pool isthread_localand initialized once -- a TTL set on a later client wouldotherwise be silently ignored.
Note
There is a separate, hand-written PR against
mainfor the same feature.It is not a cherry-pick in either direction:
mainroutes this throughnet/http/dialer.*, which does not exist on this branch. Please do not addneed-backporthere.