dsmiley commented on PR #4626: URL: https://github.com/apache/solr/pull/4626#issuecomment-5533762107
I have some doubts with this. It's suspicious to me that we're reverting that -1 default after that change itself was already figured out in a previous change. I decided to pay $5 to Fable 5 to thoroughly analyze this PR conversation and the JIRA thread. I read all of this; please do likewise. I didn't chase the references it claimed. I'm going to push a test that shows the -1 -> 60000 change breaks something that once worked. ---- I dug into the current state of this PR, in particular the unresolved question in https://github.com/apache/solr/pull/4626#discussion_r3670896438 about whether `Request.idleTimeout` overrides the global `httpClient.setIdleTimeout(...)`. Short version: it does for HTTP/1.1, but **not for HTTP/2** (our default transport), so the 60s global idle timeout added in the last commit reintroduces SOLR-17871. ## The global idle timeout caps every HTTP/2 request at 60s Jetty 12.1.10 source: * **HTTP/1.1**: `HttpConnectionOverHTTP.send()` calls `endPoint.setIdleTimeout(request.getIdleTimeout())`. The endpoint's timer is replaced for the duration of the exchange, so the per-request value really does override the global one. * **HTTP/2**: `HttpSenderOverHTTP2` calls `stream.setIdleTimeout(...)`. That only gives the *stream* its own timer. The *session/endpoint* keeps the global value. `HTTP2Session.StreamsState.onIdleTimeout()` has no "are there active streams?" check; it notifies `HTTPSessionListenerPromise.onIdleTimeout`, which delegates to `HttpConnection.onIdleTimeout`, whose `idleTimeoutGuard` only covers the brief `send()` call, not the lifetime of the exchange. It returns `true`, and the session `halt()`s all streams. This is exactly the mechanism behind the original SOLR-17871 bug, where `HTTP2Client`'s hardcoded 30s default did the capping. I confirmed it empirically on this branch with a throwaway test: a servlet that stays silent for 70s, a plain `HttpJettySolrClient` built with `.withIdleTimeout(120, SECONDS)`: ``` SolrServerException: Session idle timeout expired (after 60146ms) cause: java.util.concurrent.TimeoutException: Session idle timeout expired at org.eclipse.jetty.http2.HTTP2Session$StreamsState.newTimeoutException ``` `testIdleTimeoutWithHttpClient` doesn't catch this because it streams 10 packets × 500ms = 5s (40s nightly), both under 60s. It only ever proved the old 30s cap was gone. Note the irony given the JIRA title: anyone who raised `socketTimeout` above 60s for `SolrClientCache` would still die at 60s with this branch, just with a different exception. ## What this PR was actually chasing 1. Request timeout default → 0. Correct; that's the JIRA proposal. 2. `testTimeoutExceptionMarksServerAsZombieAsyncRequest` then hangs. The blackhole server accepts TCP but never acks the HTTP/2 preface, so no stream exists yet and the per-request idle timeout hasn't been applied. The global idle timeout is -1. The old request timeout was the only thing bounding this, and now nothing bounds the async path. 3. The sync path is masked: `listener.get(idleTimeoutMillis)` bounds the wait for headers at the application level. 4. @VishnuPriyaChandraSekar's July 22 suggestion (bound the async future the same way) was the right direction. The global idle timeout was a detour. So the real hole this PR opens is: a server that accepts a connection but never speaks HTTP/2 makes an async request hang forever. Legitimate, but narrow, and it should be closed without a global cap. ## Proposal * Restore `httpClient.setIdleTimeout(-1)` and the original comment. This is required for HTTP/2 streaming. * Bound the async pre-headers wait in `HttpJettySolrClient` to mirror the sync path: a timer of `idleTimeoutMillis` that aborts the request if `onHeaders` hasn't fired, cancelled once headers arrive. This gives both paths a consistent meaning ("idle timeout applies from send") and fixes the LB2 async test without needing the fake HTTP/2 server. * JDK client: this PR removes its *only* timeout. A sync request to a silent server now hangs forever, where before it was 10 minutes. Since the JDK's `HttpRequest.timeout` only covers time-to-response-headers, defaulting it to `idleTimeoutMillis` is harmless to streaming and keeps a safety net. Suggest `HttpJdkSolrClient.decorateRequest` fall back to the idle timeout when the request timeout is 0. * Add a regression test that streams (or stays silent) longer than any plausible global default, with a longer builder idle timeout. ## Smaller items * CI is red on all three test jobs at `compileTestJava`: `MockFakeServer` needs to be `private static class` (errorprone `ClassCanBeStatic`). * `SolrHttpConstants.DEFAULT_IDLE_TIMEOUT` (60s) sits next to `DEFAULT_SO_TIMEOUT` (600s), which is the builder's actual idle default. Confusing; moot if the global goes back to -1. * The new comment above `httpClient.setIdleTimeout(...)` states the override claim that is false for HTTP/2. Also a typo: "timeout.The". * `assertEquals(0, client.requestTimeoutMillis)` landed in an unrelated JDK test. * The changelog should mention the JDK "no timeout at all by default" consequence if that isn't changed. -- 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]
