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 00000000000..27d9870c57e --- /dev/null +++ b/changelog/unreleased/SOLR-17433-changed-default-request-timeout.yml @@ -0,0 +1,9 @@ +title: > + SolrClients no longer have a default request timeout. It was problematic with streaming expressions. + The idle timeout remains, albeit the JDK HttpClient doesn't support that (has none). +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/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java b/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java index af159f58a6a..278223035ed 100644 --- a/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java +++ b/solr/solrj-jetty/src/java/org/apache/solr/client/solrj/jetty/HttpJettySolrClient.java @@ -294,7 +294,13 @@ private HttpClient createHttpClient(Builder builder) { asyncTracker.getMaxRequestsQueuedPerDestination()); httpClient.setUserAgentField(new HttpField(HttpHeader.USER_AGENT, USER_AGENT)); httpClient.setConnectTimeout(builder.getConnectionTimeoutMillis()); - httpClient.setIdleTimeout(-1); // don't enforce an idle timeout at this level + // This global idle timeout serves as both the connection idle timeout and the default read + // timeout.The read timeout can be overridden for an individual request using Jetty's + // Request.idleTimeout(). + // Note: Do not override the global idle timeout with the HttpJettySolrClient builder's idle + // timeout since the builder's value is intended to control the read timeout for an individual + // request. See https://issues.apache.org/jira/browse/SOLR-17871 for more details. + httpClient.setIdleTimeout(SolrHttpConstants.DEFAULT_IDLE_TIMEOUT); // note: idle & request timeouts are set per request var cookieStore = builder.getCookieStore(); @@ -648,7 +654,9 @@ protected void decorateRequest(Request req, SolrRequest solrRequest, boolean h.add(CommonParams.SOLR_REQUEST_CONTEXT_PARAM, getContext().toString()); }); + // idle timeout between data transfers in a request req.idleTimeout(idleTimeoutMillis, TimeUnit.MILLISECONDS); + // hard limit on total execution time of a request req.timeout(requestTimeoutMillis, TimeUnit.MILLISECONDS); if (solrRequest.getUserPrincipal() != null) { 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 11bf3e3c85a..8f3d9a5c7fd 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 @@ -667,14 +667,34 @@ public void testBuilder() { } } + /** + * A streaming response may be silent for a long time before its first byte (e.g. a streaming + * expression that must compute before emitting anything). The builder's idle timeout must govern + * that; nothing at the Jetty HttpClient level may cap it. See SOLR-17871. + */ @Test - public void testIdleTimeoutWithHttpClient() throws Exception { + public void testIdleTimeoutBeyondOneMinute() throws Exception { String url = solrTestRule.getBaseUrl() + SLOW_STREAM_SERVLET_PATH; - try (var oldClient = + long silenceMs = 65_000; + try (var client = new HttpJettySolrClient.Builder(url) - .withRequestTimeout(Long.MAX_VALUE, TimeUnit.MILLISECONDS) - .withIdleTimeout(100, TimeUnit.MILLISECONDS) + .withIdleTimeout(silenceMs + 30_000, TimeUnit.MILLISECONDS) .build()) { + var params = new ModifiableSolrParams().set("count", "1").set("packetMs", "" + silenceMs); + QueryRequest req = new QueryRequest(params); + req.setResponseParser(new InputStreamResponseParser(FILE_STREAM)); + NamedList response = client.request(req); + try (InputStream is = (InputStream) response.get("stream")) { + assertEquals("0", new String(is.readAllBytes(), StandardCharsets.UTF_8)); + } + } + } + + @Test + public void testIdleTimeoutWithHttpClient() throws Exception { + String url = solrTestRule.getBaseUrl() + SLOW_STREAM_SERVLET_PATH; + try (var oldClient = + 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 722b241cef6..d16b54d4d39 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 @@ -491,7 +491,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 fccc4357fcd..8dd5a237860 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/java/org/apache/solr/client/solrj/impl/SolrHttpConstants.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrHttpConstants.java index 32d4fc6cbfe..c1425881b7e 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrHttpConstants.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrHttpConstants.java @@ -21,6 +21,7 @@ public interface SolrHttpConstants { int DEFAULT_CONNECT_TIMEOUT = 60000; int DEFAULT_SO_TIMEOUT = 600000; + int DEFAULT_IDLE_TIMEOUT = 60000; int DEFAULT_MAXCONNECTIONSPERHOST = 100000; int DEFAULT_MAXCONNECTIONS = 100000; 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 9cfc922c975..bf59c13137c 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,10 @@ 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 + // JDK builders does not support overriding idle timeout. Hence, overriding the default + // request timeout to prevent long hanging requests. + .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 9f9233f375e..e23835b1e51 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 @@ -311,6 +311,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); } } @@ -342,19 +344,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 849d8953c0f..89e153349ad 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 @@ -18,6 +18,8 @@ import java.io.Closeable; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.io.UncheckedIOException; import java.net.ServerSocket; import java.net.Socket; @@ -72,7 +74,6 @@ public static void beforeClass() { @Override public void setUp() throws Exception { super.setUp(); - for (int i = 0; i < solr.length; i++) { solr[i] = new SolrInstance("solr/collection1" + i, createTempDir("instance-" + i), 0); solr[i].setUp(); @@ -387,33 +388,109 @@ public void close() { } } + private class MockFakeServer implements AutoCloseable { + // Server connection preface (Empty SETTINGS frame) + static final byte[] SERVER_SETTINGS = + new byte[] { + 0x00, + 0x00, + 0x00, // Payload Length: 0 + 0x04, // Frame Type: SETTINGS + 0x00, // Flags: 0 + 0x00, + 0x00, + 0x00, + 0x00 // Stream ID: 0 + }; + + // ACK frame for client's SETTINGS + static final byte[] SETTINGS_ACK = + new byte[] { + 0x00, + 0x00, + 0x00, // Payload Length: 0 + 0x04, // Frame Type: SETTINGS + 0x01, // Flags: ACK (0x01) + 0x00, + 0x00, + 0x00, + 0x00 // Stream ID: 0 + }; + final ServerSocket serverSocket; + volatile Socket activeSocket; + + MockFakeServer() throws Exception { + serverSocket = new ServerSocket(0); + } + + int getPort() { + return this.serverSocket.getLocalPort(); + } + + /** + * The server just acknowledges client preface & settings however it never responds to client + * requests. By doing so, it causes client to hang until idle timeout configured in the request. + */ + void process() { + try (Socket clientSocket = serverSocket.accept(); + InputStream in = clientSocket.getInputStream(); + OutputStream out = clientSocket.getOutputStream()) { + this.activeSocket = clientSocket; + byte[] clientPreface = new byte[24]; + int bytesRead = in.readNBytes(clientPreface, 0, 24); + + if (bytesRead == 24) { + out.write(SERVER_SETTINGS); + out.write(SETTINGS_ACK); + out.flush(); + } + Thread.sleep(5000); + } catch (IOException | InterruptedException ignored) { + } finally { + this.activeSocket = null; + } + } + + @Override + public void close() { + try { + if (!serverSocket.isClosed()) { + serverSocket.close(); + } + if (activeSocket != null && !activeSocket.isClosed()) { + activeSocket.close(); + } + } catch (IOException ignored) { + } + } + } + private class TimeoutZombieTestContext implements AutoCloseable { - final ServerSocket blackhole; final LBSolrClient.Endpoint nonRoutableEndpoint; final HttpJettySolrClient delegateClient; final LBAsyncSolrClient lbClient; + final Thread serverThread; + final MockFakeServer mockFakeServer; TimeoutZombieTestContext() throws Exception { - // create a socket that allows a client to connect but causes them to hang until idleTimeout - // is triggered - blackhole = new ServerSocket(0); - int blackholePort = blackhole.getLocalPort(); + mockFakeServer = new MockFakeServer(); nonRoutableEndpoint = - new LBSolrClient.Endpoint("http://localhost:" + blackholePort + "/solr"); + new LBSolrClient.Endpoint("http://localhost:" + mockFakeServer.getPort() + "/solr"); delegateClient = new HttpJettySolrClient.Builder() .withConnectionTimeout(1000, TimeUnit.MILLISECONDS) .withIdleTimeout(1, TimeUnit.MILLISECONDS) .build(); - lbClient = new LBJettySolrClient.Builder(delegateClient, nonRoutableEndpoint).build(); + serverThread = new Thread(mockFakeServer::process); + serverThread.setDaemon(true); + serverThread.start(); } LBSolrClient.Req createQueryRequest() { SolrQuery solrQuery = new SolrQuery("*:*"); QueryRequest queryRequest = new QueryRequest(solrQuery); - List endpoints = List.of( new LBSolrClient.Endpoint( @@ -430,11 +507,14 @@ void assertZombieState() { @Override public void close() { - lbClient.close(); - delegateClient.close(); try { - blackhole.close(); - } catch (IOException ioe) { + if (serverThread != null) { + serverThread.interrupt(); + } + mockFakeServer.close(); + lbClient.close(); + delegateClient.close(); + } catch (Exception ignored) { } } diff --git a/solr/test-framework/src/java/org/apache/solr/util/ServletFixtures.java b/solr/test-framework/src/java/org/apache/solr/util/ServletFixtures.java index cb1b66f6eee..dfd6e5c7c5f 100644 --- a/solr/test-framework/src/java/org/apache/solr/util/ServletFixtures.java +++ b/solr/test-framework/src/java/org/apache/solr/util/ServletFixtures.java @@ -138,11 +138,13 @@ public static class SlowStreamServlet extends HttpServlet { @Override protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { String countStr = req.getParameter("count"); + String packetMsStr = req.getParameter("packetMs"); + long packetMs = packetMsStr == null ? PACKET_MS : Long.parseLong(packetMsStr); IntStream.range(0, countStr == null ? 10 : Integer.parseInt(countStr)) .forEach( i -> { try { - Thread.sleep(PACKET_MS); + Thread.sleep(packetMs); resp.getOutputStream().write(String.valueOf(i).getBytes(StandardCharsets.UTF_8)); resp.getOutputStream().flush(); } catch (IOException | InterruptedException e) {