Copilot commented on code in PR #13447:
URL: https://github.com/apache/trafficserver/pull/13447#discussion_r4162669728
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -420,21 +420,35 @@ HttpSessionManager::acquire_session(HttpSM *sm, sockaddr
const *ip, const char *
to_return = nullptr;
}
- // Otherwise, check the thread pool first
- if (this->get_pool_type() == TS_SERVER_SESSION_SHARING_POOL_THREAD ||
- this->get_pool_type() == TS_SERVER_SESSION_SHARING_POOL_HYBRID) {
- retval = _acquire_session(ip, hostname_hash, sm, match_style,
TS_SERVER_SESSION_SHARING_POOL_THREAD);
- }
-
- // If you didn't get a match, and the global pool is an option go there.
+ // Always check the thread-local pool first. Multiplexing server sessions
+ // (HTTP/2) cannot be safely shared across threads -- their state is
+ // owned by the EThread that drives their connection -- so they are filed
+ // exclusively in the per-thread pool by `Http2ServerSession::add_session`.
+ // If the configured pool type is `global`
+ // or `global_locked`, only the global pool would otherwise be consulted,
+ // which means an existing H/2 origin connection on this thread is invisible
+ // to the lookup. Each new request would then open a fresh TCP+TLS+H/2
+ // handshake to the origin, defeating multiplexing entirely. Trying the
+ // thread-local pool first restores within-thread H/2 origin reuse without
+ // changing behavior for HTTP/1.x sessions, which fall through to the
+ // configured pool below on a thread-local miss or failed allocation.
+ // Multiplexing sessions remain in the thread pool even when allocation
fails.
+ retval = _acquire_session(ip, hostname_hash, sm, match_style,
TS_SERVER_SESSION_SHARING_POOL_THREAD);
+
+ bool const thread_retry = retval == HSMresult_t::RETRY;
if (retval != HSMresult_t::DONE) {
if (TS_SERVER_SESSION_SHARING_POOL_GLOBAL == this->get_pool_type() ||
TS_SERVER_SESSION_SHARING_POOL_HYBRID == this->get_pool_type()) {
retval = _acquire_session(ip, hostname_hash, sm, match_style,
TS_SERVER_SESSION_SHARING_POOL_GLOBAL);
- } else if (TS_SERVER_SESSION_SHARING_POOL_GLOBAL_LOCKED ==
this->get_pool_type())
+ } else if (TS_SERVER_SESSION_SHARING_POOL_GLOBAL_LOCKED ==
this->get_pool_type()) {
retval = _acquire_session(ip, hostname_hash, sm, match_style,
TS_SERVER_SESSION_SHARING_POOL_GLOBAL_LOCKED);
+ }
}
+ if (thread_retry && this->get_pool_type() !=
TS_SERVER_SESSION_SHARING_POOL_THREAD) {
+ // The caller accounts for the global probe's result independently.
+ Metrics::Counter::increment(http_rsb.origin_reuse_fail);
+ }
return retval;
Review Comment:
This intermediate increment makes `origin_reuse_fail` inaccurate: the caller
already increments it when the final `acquire_session()` result is `RETRY`, so
a thread-pool `RETRY` followed by a global `RETRY` is counted twice, while a
thread-pool `RETRY` followed by a successful global reuse still records a
failure. Keep this existing metric tied to the final acquisition result, or
introduce a separate per-pool lock/contention metric.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
