dsmiley commented on PR #4626: URL: https://github.com/apache/solr/pull/4626#issuecomment-5051851249
An analysis of why the HttpJettySolrClient sets the idle timeout where it does: ## Why idle timeout is set per-request (`decorateRequest`) instead of only at the `httpClient` level The line `httpClient.setIdleTimeout(-1); // don't enforce an idle timeout at this level` (`HttpJettySolrClient.java:304`) was added in apache/solr PR [#3497](https://github.com/apache/solr/pull/3497) (commit `9052441aac9`), fixing [SOLR-17871](https://issues.apache.org/jira/browse/SOLR-17871): *"Http2SolrClient wasn't honoring idle timeout configuration above 30 seconds -- a regression."* An earlier, superseded attempt at the same fix was PR [#3493](https://github.com/apache/solr/pull/3493) (by Thomas Wöckinger), replaced so it could be rebased on `main` with better tests. ### Root cause (SOLR-17871) A prior refactor (SOLR-17776) accidentally dropped `httpClient.setIdleTimeout(idleTimeoutMillis)`. Once removed, Jetty's `HTTP2Session` fell back to its hardcoded **30s default** at the *connection/session* level — independent of whatever idle timeout was configured per-request. So any request configured with `idleTimeout > 30s` would still get killed after 30s of connection inactivity. ### Two distinct timeout mechanisms in Jetty | API | Scope | Purpose | |---|---|---| | `HttpClient#setIdleTimeout()` | connection/session | Detects a dead/inactive **connection** (used internally by `HTTP2Session`) | | `Request#idleTimeout()` | single exchange | Time between bytes for **that one request/response** | These are not interchangeable — setting only the connection-level value can't express "this particular request should tolerate N seconds of silence." ### Why per-request, not just fixing the connection-level value The underlying `httpClient` can be **shared across multiple `HttpJettySolrClient` instances**, each configured with a *different* idle timeout: - `Builder.withHttpClient()` (`HttpJettySolrClient.java:1031`), used by `requestWithBaseUrl` / `NoCloseHttpJettySolrClient`. - Test pattern in `HttpJettySolrClientTest.java:694`: ```java new HttpJettySolrClient.Builder(url) .withHttpClient(oldClient) // reuses the same underlying Jetty HttpClient .withIdleTimeout(newIdleTimeoutMs, TimeUnit.MILLISECONDS) // but wants its own timeout .build() ``` If the idle timeout were only set on the shared `httpClient`, all clients/requests sharing that pooled connection would be forced to a single global value. Setting it per-request via `decorateRequest` (`req.idleTimeout(idleTimeoutMillis, TimeUnit.MILLISECONDS)`, line 634) lets each `HttpJettySolrClient` instance apply its own configured timeout even while sharing connections. ### The resulting fix (two-part) 1. **Disable** the connection/session-level idle timeout entirely (`httpClient.setIdleTimeout(-1)`) so it can never prematurely kill a long-lived connection out from under a request with a longer configured timeout. 2. **Rely exclusively** on the per-request `req.idleTimeout(...)` set in `decorateRequest` as the single source of truth for how long to wait between bytes on a given exchange — the same mechanism that lets multiple clients sharing one pooled `httpClient` have independent idle-timeout configuration. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
