SOLR-17433: Set default http request timeout to infinite

[VishnuPriyaChandraSekar]

Description

In earlier versions of SolrJ 9.4, the legacy HttpSolrClient was able to support long running streaming operation (> 10 mins). However, in SolrJ 9.4+, the http client throws exception exactly after 10 mins.

https://issues.apache.org/jira/browse/SOLR-17433

Solution

In case of long streaming operation, the server sends the response in chunks and request timeout determines how long the client can wait to receive all the chunks. The default request timeout was set to 10 mins which caused the streaming operations to timeout abruptly.

In order to support long running streams, the request timeout must be infinite (as we don't know how long streams will take to complete). Jetty excepts either 0 or -1 however JDK expects no value to represent the infinite timeout. The following changes were made to implement the behavior

• HttpSolrClient: Set the default request timeout to infinite by setting it to zero. This helps streams to continue streaming until they exhaust the content.
• HttpJdkSolrClient: Prevented passing over the request timeout to JDK's HttpClient if it is zero.

Tests

I observed that the idle timeout, overridden through HttpSolrJettyClient.Builder is applied only after receiving the header response from server. Meanwhile, the hard code idle timeout (-1) is applied. This causes some of the tests to hang infinitely. To prevent this, changed the default idle timeout in Jetty Client to 10 mins.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• [X ] I have added tests for my changes.
• I have added documentation for the Reference Guide
• I have added a changelog entry for my change

[dsmiley]

This should probably just be 0, infinite?

[VishnuPriyaChandraSekar]

By default the request timeout is infinite which causes the test to hang forever. Thus, I had to change it to a definite value.

[dsmiley]

something seems suspicious... the idle timeout should work if the request timeout is infinite. They are different things; idle refers to the longest period of not receiving any data at all from the server. The request timeout is an overall timeout for the entire request.

[VishnuPriyaChandraSekar]

The idle timeout is ineffective for this test. The http client (through Jetty) uses HTTP2 protocol to communicate with the server. After establishing connection with the server, it tries to send the request headers to the server as part of creating HTTP streams. However, the fake server fails to ack the headers.
The client is waiting infinitely for the server to ack and the connection never times out because of following reasons

• The idle timeout in the request (1ms) is not applied as the Jetty applies it only after receiving ack from the server [1]. The default idle timeout (-1) set in the HttpJettySolrClient is applied [2]
• The default request timeout was changed to -1

I was wondering how the test worked before. It looks like the test wanted to simulate socket timeout, however it was not happening. The HTTP request was timing out because of request timeout (previously it took idle timeout as default value.)

In order to fix the test, either the fake server must ack the header (Gemini says it is possible however I haven't tried first hand) or simulate request timeout instead. Let me know which one choose

Thanks to the Jetty logs [3] for helping me to understand this.

[1] https://github.com/jetty/jetty.project/blob/jetty-12.1.x/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpSenderOverHTTP2.java#L203
[2] https://github.com/apache/solr/blob/main/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java#L304
[3] Jetty logs: https://gist.github.com/VishnuPriyaChandraSekar/0e3d428cbfdd7025c18562356af7da4d

[dsmiley]

RE "this test": LB2SolrClientTest is a test suite with a number of tests, and this comment thread is on a general part of the test suite. Hopefully the "connect" timeout ones are valid; yes? So I suppose your input is that the "testTimeoutExceptionMarksServerAsZombie" test and Async one are flawed?

If the default idle timeout of -1 is actually used as such for the first response, even in the beginning... this is a problem IMO. If we have the intended/desired idle timeout then we can use that. If we don't, I suppose the connection timeout would be a reasonable substitute.
Obviously what we do will need some comments to help anyone studying the internals understand.

[VishnuPriyaChandraSekar]

"this test": LB2SolrClientTest is a test suite with a number of tests, and this comment thread is on a general part of the test suite. Hopefully the "connect" timeout ones are valid; yes? So I suppose your input is that the "testTimeoutExceptionMarksServerAsZombie" test and Async one are flawed?

@dsmiley The issue only affects testTimeoutExceptionMarksServerAsZombieAsyncRequest. Interestingly, the synchronous counterpart, testTimeoutExceptionMarksServerAsZombie, does throw a TimeoutException.

After digging into the code, it looks like the difference is that the synchronous API waits for the response headers using listener.get(idleTimeoutMillis, TimeUnit.MILLISECONDS), where idleTimeoutMillis comes from the request rather than the default infinite idle timeout set at transport layer [1]. As a result, even though the transport layer is configured with an infinite idle timeout, the application-layer timeout prevents the request from hanging indefinitely.

The async API, on the other hand, doesn't have an equivalent application-layer timeout and instead relies on the transport layer to trigger onHeaders() or onFailure() [2]. Since the transport idle timeout isn't applied at this stage, the future never completes and the test hangs.

[1] https://github.com/apache/solr/blob/main/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java#L484
[2] https://github.com/apache/solr/blob/main/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java#L402

If the default idle timeout of -1 is actually used as such for the first response, even in the beginning... this is a problem IMO. If we have the intended/desired idle timeout then we can use that. If we don't, I suppose the connection timeout would be a reasonable substitute.

Maybe we should a have timeout at the application layer like the sync call ? (Not sure whether it is the right PR to fix the async call as it seem to be a different problem)

Apologizes, if my findings weren't complete earlier. I found it hard to debug as the timeout was triggered at different layer based on the request and also tracing Java futures was hard. I hope now it make sense.

[VishnuPriyaChandraSekar]

I think, it makes sense to set the idle timeout to a definite value. The problem will be solved without needing to add timeout to the futures that handles async request.

[dsmiley]

Strangely... it seems this PR/branch now has several individual commits copied from main branch. If you intended to resync this PR with main, you simply merge main into your feature branch. To remedy this, simply do that now.

[dsmiley]

I checked out the PR and made 2 minor changes (see commit messages for each). I was focused on the Jetty client at first, as it's the most important client.

Then I proceeded to look at the JDK side closer. Then I found this: https://stackoverflow.com/questions/64550136/how-to-set-socket-timeout-in-java-http-client#:~:text=The%20HttpClient%20lets%20you%20set,the%20reception%20of%20the%20body.

Wow... so it appears JDK doesn't actually implement a request timeout as defined by Jetty HttpClient & Apache HttpClient. The JDK's supposed request timeout is for when the headers have been returned. And there's no socket idle timeout support at all. If this is true (and it should be confirmed from additional authoritative sources!) then what we call the idle timeout would loosely map to the JDK's request timeout, albeit somewhat inferior. And supporting a real request timeout would need to be on top of the JDK HttpClient, thus in HttpJdkSolrClient.

We should ensure our builder javadocs are extra clear on these things so users understand what they are setting. We tried to be generic but JDK in particular will have a quirk with idle's definition.

[dsmiley]

I really appreciate your investigation and digging in here! This issue turned out to be harder than it appeared initially on the surface.

[dsmiley]

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 (commit 9052441aac9), fixing 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 (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:

  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.

[dsmiley]

I suppose we should consider this an experiment that the build may prove out? We want to make it possible to choose a longer idle timeout in the builder, I'd think. If a longer one is chosen, it won't work; it'll be effectively capped at this amount. On the other hand... if this works great except for this limitation, and doesn't seem like a big limitation, then maybe this is the price to pay.

Also... the comment you removed is still partially relevant.