diff --git a/changelog/unreleased/SOLR-17433-changed-default-request-timeout.yml b/changelog/unreleased/SOLR-17433-changed-default-request-timeout.yml new file mode 100644 index 000000000000..e7821271e8dc --- /dev/null +++ b/changelog/unreleased/SOLR-17433-changed-default-request-timeout.yml @@ -0,0 +1,7 @@ +title: Changed default http request timeout to infinite +type: changed +authors: + - name: Vishnu Priya Chandra Sekar +links: + - name: SOLR-17433 + url: https://issues.apache.org/jira/browse/SOLR-17433 diff --git a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/ConcurrentUpdateJettySolrClientTest.java b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/ConcurrentUpdateJettySolrClientTest.java index df28d17f3b0d..50d568d53f1e 100644 --- a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/ConcurrentUpdateJettySolrClientTest.java +++ b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/ConcurrentUpdateJettySolrClientTest.java @@ -47,7 +47,8 @@ public ConcurrentUpdateBaseSolrClient outcomeCountingConcurrentClient( public HttpSolrClient solrClient(Integer overrideIdleTimeoutMs) { var builder = new HttpJettySolrClient.Builder(); if (overrideIdleTimeoutMs != null) { - builder.withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS); + builder + .withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS); } return builder.build(); } diff --git a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java index 11bf3e3c85a2..6ba37acc49f7 100644 --- a/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java +++ b/solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientTest.java @@ -671,10 +671,7 @@ public void testBuilder() { public void testIdleTimeoutWithHttpClient() throws Exception { String url = solrTestRule.getBaseUrl() + SLOW_STREAM_SERVLET_PATH; try (var oldClient = - new HttpJettySolrClient.Builder(url) - .withRequestTimeout(Long.MAX_VALUE, TimeUnit.MILLISECONDS) - .withIdleTimeout(100, TimeUnit.MILLISECONDS) - .build()) { + new HttpJettySolrClient.Builder(url).withIdleTimeout(100, TimeUnit.MILLISECONDS).build()) { try (var onlyBaseUrlChangedClient = new HttpJettySolrClient.Builder(url).withHttpClient(oldClient).build()) { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index 1f88adc519fd..b9c9f26cc6a8 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java @@ -448,7 +448,10 @@ private synchronized boolean maybeTryHeadRequestSync(URI uriNoQueryParams) { } private void decorateRequest(HttpRequest.Builder reqb, SolrRequest solrRequest) { - reqb.timeout(Duration.of(requestTimeoutMillis, ChronoUnit.MILLIS)); + // JDK does not allow non-positive value for request timeout. + if (requestTimeoutMillis > 0) { + reqb.timeout(Duration.of(requestTimeoutMillis, ChronoUnit.MILLIS)); + } reqb.header("User-Agent", USER_AGENT); setBasicAuthHeader(solrRequest, reqb); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index fccc4357fcde..8dd5a237860c 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -580,10 +580,16 @@ public B withRequestTimeout(long requestTimeout, TimeUnit unit) { return (B) this; } + /** + * Returns the request timeout in milliseconds. + * + *

If no request timeout is configured or if the configured value is non-positive, this + * method returns {@code 0}, indicating that requests will wait infinitely for a response. + * + * @return request timeout in milliseconds or {@code 0} if no valid timeout is configured. + */ public long getRequestTimeoutMillis() { - return requestTimeoutMillis != null && requestTimeoutMillis > 0 - ? requestTimeoutMillis - : getIdleTimeoutMillis(); + return requestTimeoutMillis != null && requestTimeoutMillis > 0 ? requestTimeoutMillis : 0; } /** diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java index 13e790b60b2b..259e8180d1b1 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java @@ -28,7 +28,11 @@ public HttpSolrClient solrClient(Integer overrideIdleTimeoutMs) { var builder = new HttpJdkSolrClient.Builder().withSSLContext(MockTrustManager.ALL_TRUSTING_SSL_CONTEXT); if (overrideIdleTimeoutMs != null) { - builder.withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS); + builder + .withIdleTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS) + // override the infinite request timeout with idle timeout to ensure idle requests times + // out. + .withRequestTimeout(overrideIdleTimeoutMs, TimeUnit.MILLISECONDS); } return builder.build(); } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java index 73ff00474bb1..0727506d8e16 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java @@ -175,6 +175,8 @@ public void testRequestWithBaseUrl() throws Exception { client.requestWithBaseUrl(intendedUrl, new QueryRequest(q, SolrRequest.METHOD.GET), null); assertEquals( client.getParser().getWriterType(), DebugServlet.parameters.get(CommonParams.WT)[0]); + // verify whether the default request timeout is infinite. + assertEquals(0, client.requestTimeoutMillis); } } @@ -206,19 +208,6 @@ public void testAsyncException() throws Exception { super.testAsyncExceptionBase(); } - @Test - public void testTimeout() throws Exception { - SolrQuery q = new SolrQuery("*:*"); - try (HttpJdkSolrClient client = - (HttpJdkSolrClient) - builder(solrTestRule.getBaseUrl() + SLOW_SERVLET_PATH, 500, 500).build()) { - client.query(q, SolrRequest.METHOD.GET); - fail("No exception thrown."); - } catch (SolrServerException e) { - assertTrue(e.getMessage().contains("timeout") || e.getMessage().contains("Timeout")); - } - } - @Test public void test0IdleTimeout() throws Exception { SolrQuery q = new SolrQuery("*:*"); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LB2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LB2SolrClientTest.java index 849d8953c0fa..28c757c3291e 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LB2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LB2SolrClientTest.java @@ -405,6 +405,9 @@ private class TimeoutZombieTestContext implements AutoCloseable { new HttpJettySolrClient.Builder() .withConnectionTimeout(1000, TimeUnit.MILLISECONDS) .withIdleTimeout(1, TimeUnit.MILLISECONDS) + // override the infinite request timeout with idle timeout to ensure idle requests + // times out. + .withRequestTimeout(1, TimeUnit.MILLISECONDS) .build(); lbClient = new LBJettySolrClient.Builder(delegateClient, nonRoutableEndpoint).build();