diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 9457ebce1da..87665948bdc 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -250,6 +250,13 @@ Improvements * SOLR-16951: Add PKI Auth Caching for both generation of the PKI Auth Tokens and validation of received tokens. The default PKI Auth validity TTL has been increased from 5 seconds to 10 seconds. (Houston Putman) +* SOLR-17776: SolrJ: If Http2SolrClient.Builder.withHttpClient is used, (and it's used more in 9.8, shifted from deprecated Apache HttpClient + based clients), settings affecting HttpClient construction cannot be customized; an IllegalStateException will now be thrown if you try. + The connection timeout cannot be customized, but idle & request timeouts can be. Callers (various places in Solr) no longer customize the connection timeout. + (David Smiley, Luke Kot-Zaniewski) + +* SOLR-17776: The HttpJdkSolrClient wasn't setting the connection timeout as per the builder configuration. (David Smiley) + Optimizations --------------------- * SOLR-17578: Remove ZkController internal core supplier, for slightly faster reconnection after Zookeeper session loss. (Pierre Salagnac) diff --git a/solr/core/src/java/org/apache/solr/cloud/Overseer.java b/solr/core/src/java/org/apache/solr/cloud/Overseer.java index 12b2cfa7206..91877fe790e 100644 --- a/solr/core/src/java/org/apache/solr/cloud/Overseer.java +++ b/solr/core/src/java/org/apache/solr/cloud/Overseer.java @@ -862,7 +862,6 @@ private void doCompatCheck(BiConsumer consumer) { new Http2SolrClient.Builder() .withHttpClient(getCoreContainer().getDefaultHttpSolrClient()) .withIdleTimeout(30000, TimeUnit.MILLISECONDS) - .withConnectionTimeout(15000, TimeUnit.MILLISECONDS) .build(); var client = new CloudHttp2SolrClient.Builder( diff --git a/solr/core/src/java/org/apache/solr/cloud/SyncStrategy.java b/solr/core/src/java/org/apache/solr/cloud/SyncStrategy.java index 2f9a3248b6b..a686743f6b5 100644 --- a/solr/core/src/java/org/apache/solr/cloud/SyncStrategy.java +++ b/solr/core/src/java/org/apache/solr/cloud/SyncStrategy.java @@ -360,7 +360,6 @@ private void requestRecovery( try (SolrClient client = new Http2SolrClient.Builder(baseUrl) .withHttpClient(solrClient) - .withConnectionTimeout(30000, TimeUnit.MILLISECONDS) .withIdleTimeout(120000, TimeUnit.MILLISECONDS) .build()) { client.request(recoverRequestCmd); diff --git a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java index 80c4ba6f1f9..eb5cf2f655f 100644 --- a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java +++ b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java @@ -179,8 +179,6 @@ public class IndexFetcher { private final Http2SolrClient solrClient; - private Integer connTimeout; - private Integer soTimeout; private boolean skipCommitOnLeaderVersionZero = true; @@ -258,7 +256,6 @@ private Http2SolrClient createSolrClient( .withHttpClient(updateShardHandler.getRecoveryOnlyHttpClient()) .withBasicAuthCredentials(httpBasicAuthUser, httpBasicAuthPassword) .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS) - .withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS) .build(); } @@ -287,7 +284,6 @@ public IndexFetcher( String compress = (String) initArgs.get(COMPRESSION); useInternalCompression = ReplicationHandler.INTERNAL.equals(compress); useExternalCompression = ReplicationHandler.EXTERNAL.equals(compress); - connTimeout = getParameter(initArgs, HttpClientUtil.PROP_CONNECTION_TIMEOUT, 30000, null); soTimeout = getParameter(initArgs, HttpClientUtil.PROP_SO_TIMEOUT, 120000, null); String httpBasicAuthUser = (String) initArgs.get(HttpClientUtil.PROP_BASIC_AUTH_USER); diff --git a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc index f9aca44cac0..52fe51678d1 100644 --- a/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc +++ b/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc @@ -67,6 +67,14 @@ It is always strongly recommended that you fully reindex your documents after a In Solr 8, it was possible to add docValues to a schema without re-indexing via `UninvertDocValuesMergePolicy`, an advanced/expert utility. Due to changes in Lucene 9, that isn't possible any more. +== Solr 9.9 + +=== SolrJ + +If Http2SolrClient.Builder.withHttpClient is used, (and it's used more in 9.8, shifted from deprecated Apache HttpClient based clients), settings affecting HttpClient construction cannot be customized; an IllegalStateException will now be thrown if you try. +The connection timeout cannot be customized, but idle & request timeouts can be. +Callers (various places in Solr) no longer customize the connection timeout. + == Solr 9.8.1 === Configuration diff --git a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java index 7550d1a35c4..7e0756a0fa8 100644 --- a/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java +++ b/solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/SolrClientCache.java @@ -205,19 +205,14 @@ private static Http2SolrClient.Builder newHttp2SolrClientBuilder( .withDefaultCollection(URLUtil.extractCoreFromCoreUrl(url)); if (http2SolrClient != null) { builder.withHttpClient(http2SolrClient); + // cannot set connection timeout + } else { + builder.withConnectionTimeout(minConnTimeout, TimeUnit.MILLISECONDS); } + builder.withIdleTimeout( + Math.max(minSocketTimeout, builder.getIdleTimeoutMillis()), TimeUnit.MILLISECONDS); builder.withOptionalBasicAuthCredentials(basicAuthCredentials); - long idleTimeout = minSocketTimeout; - if (builder.getIdleTimeoutMillis() != null) { - idleTimeout = Math.max(idleTimeout, builder.getIdleTimeoutMillis()); - } - builder.withIdleTimeout(idleTimeout, TimeUnit.MILLISECONDS); - long connTimeout = minConnTimeout; - if (builder.getConnectionTimeout() != null) { - connTimeout = Math.max(idleTimeout, builder.getConnectionTimeout()); - } - builder.withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS); return builder; } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java index 4c24aa03cc0..58f59eb8eaf 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateHttp2SolrClient.java @@ -255,6 +255,7 @@ void sendUpdateStream() throws Exception { responseListener = out.getResponseListener(); } + // just wait for the headers, so the idle timeout is sensible Response response = responseListener.get(client.getIdleTimeout(), TimeUnit.MILLISECONDS); rspBody = responseListener.getInputStream(); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java index 14dc22fae42..060d75290da 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java @@ -94,15 +94,12 @@ import org.slf4j.MDC; /** - * Difference between this {@link Http2SolrClient} and {@link HttpSolrClient}: + * An HTTP {@link SolrClient} using Jetty {@link HttpClient}. This is Solr's most mature client for + * direct HTTP. * - * + *

Despite the name, this client supports HTTP 1.1 and 2 -- toggle with {@link + * HttpSolrClientBuilderBase#useHttp1_1(boolean)}. In retrospect, the name should have been {@code + * HttpJettySolrClient}. */ public class Http2SolrClient extends HttpSolrClientBase { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); @@ -114,6 +111,8 @@ public class Http2SolrClient extends HttpSolrClientBase { private final HttpClient httpClient; + private final long idleTimeoutMillis; + private List listenerFactory = new ArrayList<>(); protected AsyncTracker asyncTracker = new AsyncTracker(); @@ -129,6 +128,20 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) { super(serverBaseUrl, builder); if (builder.httpClient != null) { + // Validate that no conflicting options are provided when using an existing HttpClient + if (builder.followRedirects != null + || builder.connectionTimeoutMillis != null + || builder.maxConnectionsPerHost != null + || builder.useHttp1_1 + != builder.httpClient.getTransport() instanceof HttpClientTransportOverHTTP + || builder.proxyHost != null + || builder.sslConfig != null + || builder.cookieStore != null + || builder.keyStoreReloadIntervalSecs != null) { + throw new IllegalArgumentException( + "You cannot provide the HttpClient and also specify options that are used to build a new client"); + } + this.httpClient = builder.httpClient; if (this.executor == null) { this.executor = builder.executor; @@ -144,8 +157,19 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) { this.listenerFactory.addAll(builder.listenerFactory); } updateDefaultMimeTypeForParser(); - this.httpClient.setFollowRedirects(Boolean.TRUE.equals(builder.followRedirects)); + this.idleTimeoutMillis = builder.getIdleTimeoutMillis(); + + try { + applyHttpClientBuilderFactory(); + } catch (RuntimeException e) { + try { + this.close(); + } catch (Exception exceptionOnClose) { + e.addSuppressed(exceptionOnClose); + } + throw e; + } assert ObjectReleaseTracker.track(this); } @@ -158,6 +182,29 @@ private void initAuthStoreFromExistingClient(HttpClient httpClient) { this.authenticationStore = (AuthenticationStoreHolder) httpClient.getAuthenticationStore(); } + private void applyHttpClientBuilderFactory() { + String factoryClassName = + System.getProperty(HttpClientUtil.SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY); + if (factoryClassName != null) { + log.debug("Using Http Builder Factory: {}", factoryClassName); + HttpClientBuilderFactory factory; + try { + factory = + Class.forName(factoryClassName) + .asSubclass(HttpClientBuilderFactory.class) + .getDeclaredConstructor() + .newInstance(); + } catch (InstantiationException + | IllegalAccessException + | ClassNotFoundException + | InvocationTargetException + | NoSuchMethodException e) { + throw new RuntimeException("Unable to instantiate " + Http2SolrClient.class.getName(), e); + } + factory.setup(this); + } + } + @Deprecated(since = "9.7") public void addListenerFactory(HttpListenerFactory factory) { this.listenerFactory.add(factory); @@ -174,8 +221,6 @@ ProtocolHandlers getProtocolHandlers() { } private HttpClient createHttpClient(Builder builder) { - HttpClient httpClient; - executor = builder.executor; if (executor == null) { BlockingArrayQueue queue = new BlockingArrayQueue<>(256, 256); @@ -187,21 +232,24 @@ private HttpClient createHttpClient(Builder builder) { shutdownExecutor = false; } - SslContextFactory.Client sslContextFactory; - if (builder.sslConfig == null) { - sslContextFactory = getDefaultSslContextFactory(); - } else { - sslContextFactory = builder.sslConfig.createClientContextFactory(); - } + SSLConfig sslConfig = + builder.sslConfig != null ? builder.sslConfig : Http2SolrClient.defaultSSLConfig; + SslContextFactory.Client sslContextFactory = + (sslConfig == null) + ? getDefaultSslContextFactory() + : sslConfig.createClientContextFactory(); + Long keyStoreReloadIntervalSecs = builder.keyStoreReloadIntervalSecs; + if (keyStoreReloadIntervalSecs == null && Boolean.getBoolean("solr.keyStoreReload.enabled")) { + keyStoreReloadIntervalSecs = Long.getLong("solr.jetty.sslContext.reload.scanInterval", 30); + } if (sslContextFactory != null && sslContextFactory.getKeyStoreResource() != null - && builder.keyStoreReloadIntervalSecs != null - && builder.keyStoreReloadIntervalSecs > 0) { + && keyStoreReloadIntervalSecs != null + && keyStoreReloadIntervalSecs > 0) { scanner = new KeyStoreScanner(sslContextFactory); try { - scanner.setScanInterval( - (int) Math.min(builder.keyStoreReloadIntervalSecs, Integer.MAX_VALUE)); + scanner.setScanInterval((int) Math.min(keyStoreReloadIntervalSecs, Integer.MAX_VALUE)); scanner.start(); if (log.isDebugEnabled()) { log.debug("Key Store Scanner started"); @@ -223,6 +271,7 @@ private HttpClient createHttpClient(Builder builder) { clientConnector.setSslContextFactory(sslContextFactory); clientConnector.setSelectors(2); + HttpClient httpClient; HttpClientTransport transport; if (builder.useHttp1_1) { if (log.isDebugEnabled()) { @@ -252,17 +301,17 @@ private HttpClient createHttpClient(Builder builder) { httpClient.setMaxRequestsQueuedPerDestination( asyncTracker.getMaxRequestsQueuedPerDestination()); httpClient.setUserAgentField(new HttpField(HttpHeader.USER_AGENT, USER_AGENT)); - httpClient.setIdleTimeout(idleTimeoutMillis); + httpClient.setConnectTimeout(builder.getConnectionTimeoutMillis()); + // note: idle & request timeouts are set per request - if (builder.cookieStore != null) { - httpClient.setHttpCookieStore(builder.cookieStore); + var cookieStore = builder.getCookieStore(); + if (cookieStore != null) { + httpClient.setHttpCookieStore(cookieStore); } this.authenticationStore = new AuthenticationStoreHolder(); httpClient.setAuthenticationStore(this.authenticationStore); - httpClient.setConnectTimeout(builder.connectionTimeoutMillis); - setupProxy(builder, httpClient); try { @@ -285,15 +334,14 @@ private void setupProxy(Builder builder, HttpClient httpClient) { if (builder.proxyIsSocks4) { proxy = new Socks4Proxy(address, builder.proxyIsSecure); } else { - final Protocol protocol; - if (builder.useHttp1_1) { - protocol = HttpClientTransportOverHTTP.HTTP11; - } else { - // see HttpClientTransportOverHTTP2#newOrigin - String protocolName = builder.proxyIsSecure ? "h2" : "h2c"; - protocol = new Protocol(List.of(protocolName), false); - } - proxy = new HttpProxy(address, builder.proxyIsSecure, protocol); + // Move protocol initialization closer to where it's used + proxy = + new HttpProxy( + address, + builder.proxyIsSecure, + builder.useHttp1_1 + ? HttpClientTransportOverHTTP.HTTP11 + : new Protocol(List.of(builder.proxyIsSecure ? "h2" : "h2c"), false)); } httpClient.getProxyConfiguration().addProxy(proxy); } @@ -329,6 +377,11 @@ public void setAuthenticationStore(AuthenticationStore authenticationStore) { this.authenticationStore.updateAuthenticationStore(authenticationStore); } + /** (visible for testing) */ + public long getIdleTimeout() { + return idleTimeoutMillis; + } + public static class OutStream implements Closeable { private final String origCollection; private final SolrParams origParams; @@ -514,6 +567,7 @@ public NamedList request(SolrRequest solrRequest, String collection) try { InputStreamResponseListener listener = new InputStreamReleaseTrackingResponseListener(); req = sendRequest(makeRequest(solrRequest, url, false), listener); + // only waits for headers, so use the idle timeout Response response = listener.get(idleTimeoutMillis, TimeUnit.MILLISECONDS); url = req.getURI().toString(); InputStream is = listener.getInputStream(); @@ -589,13 +643,18 @@ private NamedList processErrorsAndResponse( ResponseParser parser = solrRequest.getResponseParser() == null ? this.parser : solrRequest.getResponseParser(); String contentType = response.getHeaders().get(HttpHeader.CONTENT_TYPE); + + // Move variable initializations closer to where they are used + String responseMethod = response.getRequest() == null ? "" : response.getRequest().getMethod(); + + // Initialize mimeType and encoding only when needed String mimeType = null; String encoding = null; if (contentType != null) { mimeType = MimeTypes.getContentTypeWithoutCharset(contentType); encoding = MimeTypes.getCharsetFromContentType(contentType); } - String responseMethod = response.getRequest() == null ? "" : response.getRequest().getMethod(); + return processErrorsAndResponse( response.getStatus(), response.getReason(), @@ -621,12 +680,9 @@ private void setBasicAuthHeader(SolrRequest solrRequest, Request req) { private void decorateRequest(Request req, SolrRequest solrRequest, boolean isAsync) { req.headers(headers -> headers.remove(HttpHeader.ACCEPT_ENCODING)); + req.idleTimeout(idleTimeoutMillis, TimeUnit.MILLISECONDS); + req.timeout(requestTimeoutMillis, TimeUnit.MILLISECONDS); - if (requestTimeoutMillis > 0) { - req.timeout(requestTimeoutMillis, TimeUnit.MILLISECONDS); - } else { - req.timeout(idleTimeoutMillis, TimeUnit.MILLISECONDS); - } if (solrRequest.getUserPrincipal() != null) { req.attribute(REQ_PRINCIPAL_KEY, solrRequest.getUserPrincipal()); } @@ -691,22 +747,26 @@ private MakeRequestReturnValue makeRequest( if (SolrRequest.METHOD.POST == solrRequest.getMethod() || SolrRequest.METHOD.PUT == solrRequest.getMethod()) { - RequestWriter.ContentWriter contentWriter = requestWriter.getContentWriter(solrRequest); - Collection streams = - contentWriter == null ? requestWriter.getContentStreams(solrRequest) : null; - - boolean isMultipart = isMultipart(streams); - + // Move method initialization closer to where it's used HttpMethod method = SolrRequest.METHOD.POST == solrRequest.getMethod() ? HttpMethod.POST : HttpMethod.PUT; + RequestWriter.ContentWriter contentWriter = requestWriter.getContentWriter(solrRequest); + if (contentWriter != null) { var content = new OutputStreamRequestContent(contentWriter.getContentType()); var r = httpClient.newRequest(url + wparams.toQueryString()).method(method).body(content); decorateRequest(r, solrRequest, isAsync); return new MakeRequestReturnValue(r, contentWriter, content); + } + + // Only get streams if contentWriter is null + Collection streams = requestWriter.getContentStreams(solrRequest); - } else if (streams == null || isMultipart) { + // Move isMultipart initialization closer to where it's used + boolean isMultipart = isMultipart(streams); + + if (streams == null || isMultipart) { // send server list and request list as query string params ModifiableSolrParams queryParams = calculateQueryParams(this.urlParamNames, wparams); queryParams.add(calculateQueryParams(solrRequest.getQueryParams(), wparams)); @@ -764,14 +824,18 @@ private Request fillContentStream( } if (streams != null) { for (ContentStream contentStream : streams) { + // Move contentType initialization closer to where it's used String contentType = contentStream.getContentType(); if (contentType == null) { contentType = "multipart/form-data"; // default } + + // Move name initialization closer to where it's used String name = contentStream.getName(); if (name == null) { name = ""; } + HttpFields.Mutable fields = HttpFields.build(1); fields.add(HttpHeader.CONTENT_TYPE, contentType); content.addPart( @@ -786,6 +850,7 @@ private Request fillContentStream( } } else { // application/x-www-form-urlencoded + // Move queryString initialization into the else block where it's used String queryString = wparams.toQueryString(); // remove the leading "?" if there is any queryString = queryString.startsWith("?") ? queryString.substring(1) : queryString; @@ -999,7 +1064,10 @@ public Http2SolrClient.Builder requestTimeout(int requestTimeout) { return this; } - private HttpCookieStore getDefaultCookieStore() { + private HttpCookieStore getCookieStore() { + if (cookieStore == null) { + return cookieStore; + } if (Boolean.getBoolean("solr.http.disableCookies")) { return new HttpCookieStore.Empty(); } @@ -1016,65 +1084,7 @@ protected B build(Class type) { @Override public Http2SolrClient build() { - if (sslConfig == null) { - sslConfig = Http2SolrClient.defaultSSLConfig; - } - if (cookieStore == null) { - cookieStore = getDefaultCookieStore(); - } - if (idleTimeoutMillis == null || idleTimeoutMillis <= 0) { - idleTimeoutMillis = (long) HttpClientUtil.DEFAULT_SO_TIMEOUT; - } - if (connectionTimeoutMillis == null) { - connectionTimeoutMillis = (long) HttpClientUtil.DEFAULT_CONNECT_TIMEOUT; - } - - if (keyStoreReloadIntervalSecs != null - && keyStoreReloadIntervalSecs > 0 - && this.httpClient != null) { - log.warn("keyStoreReloadIntervalSecs can't be set when using external httpClient"); - keyStoreReloadIntervalSecs = null; - } else if (keyStoreReloadIntervalSecs == null - && this.httpClient == null - && Boolean.getBoolean("solr.keyStoreReload.enabled")) { - keyStoreReloadIntervalSecs = Long.getLong("solr.jetty.sslContext.reload.scanInterval", 30); - } - - Http2SolrClient client = new Http2SolrClient(baseSolrUrl, this); - try { - httpClientBuilderSetup(client); - } catch (RuntimeException e) { - try { - client.close(); - } catch (Exception exceptionOnClose) { - e.addSuppressed(exceptionOnClose); - } - throw e; - } - return client; - } - - private void httpClientBuilderSetup(Http2SolrClient client) { - String factoryClassName = - System.getProperty(HttpClientUtil.SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY); - if (factoryClassName != null) { - log.debug("Using Http Builder Factory: {}", factoryClassName); - HttpClientBuilderFactory factory; - try { - factory = - Class.forName(factoryClassName) - .asSubclass(HttpClientBuilderFactory.class) - .getDeclaredConstructor() - .newInstance(); - } catch (InstantiationException - | IllegalAccessException - | ClassNotFoundException - | InvocationTargetException - | NoSuchMethodException e) { - throw new RuntimeException("Unable to instantiate " + Http2SolrClient.class.getName(), e); - } - factory.setup(client); - } + return new Http2SolrClient(baseSolrUrl, this); } /** @@ -1087,18 +1097,15 @@ public Builder withHttpClient(Http2SolrClient http2SolrClient) { if (this.basicAuthAuthorizationStr == null) { this.basicAuthAuthorizationStr = http2SolrClient.basicAuthAuthorizationStr; } - if (this.followRedirects == null) { - this.followRedirects = http2SolrClient.httpClient.isFollowRedirects(); - } if (this.idleTimeoutMillis == null) { this.idleTimeoutMillis = http2SolrClient.idleTimeoutMillis; } - if (this.requestWriter == null) { - this.requestWriter = http2SolrClient.requestWriter; - } if (this.requestTimeoutMillis == null) { this.requestTimeoutMillis = http2SolrClient.requestTimeoutMillis; } + if (this.requestWriter == null) { + this.requestWriter = http2SolrClient.requestWriter; + } if (this.responseParser == null) { this.responseParser = http2SolrClient.parser; } @@ -1106,8 +1113,7 @@ public Builder withHttpClient(Http2SolrClient http2SolrClient) { this.urlParamNames = http2SolrClient.urlParamNames; } if (this.listenerFactory == null) { - this.listenerFactory = new ArrayList(); - http2SolrClient.listenerFactory.forEach(this.listenerFactory::add); + this.listenerFactory = new ArrayList<>(http2SolrClient.listenerFactory); } if (this.executor == null) { this.executor = http2SolrClient.executor; 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 c72708b39e7..3a3160da496 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 @@ -86,12 +86,18 @@ public class HttpJdkSolrClient extends HttpSolrClientBase { protected HttpJdkSolrClient(String serverBaseUrl, HttpJdkSolrClient.Builder builder) { super(serverBaseUrl, builder); + HttpClient.Builder b = HttpClient.newBuilder(); HttpClient.Redirect followRedirects = Boolean.TRUE.equals(builder.followRedirects) ? HttpClient.Redirect.NORMAL : HttpClient.Redirect.NEVER; - HttpClient.Builder b = HttpClient.newBuilder().followRedirects(followRedirects); + b.followRedirects(followRedirects); + + b.connectTimeout(Duration.of(builder.getConnectionTimeoutMillis(), ChronoUnit.MILLIS)); + // note: idle timeout isn't used for the JDK client + // note: request timeout is set per request + if (builder.sslContext != null) { b.sslContext(builder.sslContext); } @@ -419,11 +425,7 @@ private synchronized boolean maybeTryHeadRequestSync(String url) { } private void decorateRequest(HttpRequest.Builder reqb, SolrRequest solrRequest) { - if (requestTimeoutMillis > 0) { - reqb.timeout(Duration.of(requestTimeoutMillis, ChronoUnit.MILLIS)); - } else if (idleTimeoutMillis > 0) { - reqb.timeout(Duration.of(idleTimeoutMillis, ChronoUnit.MILLIS)); - } + reqb.timeout(Duration.of(requestTimeoutMillis, ChronoUnit.MILLIS)); reqb.header("User-Agent", USER_AGENT); setBasicAuthHeader(solrRequest, reqb); Map headers = solrRequest.getHeaders(); @@ -553,12 +555,6 @@ public Builder(String baseSolrUrl) { @Override public HttpJdkSolrClient build() { - if (idleTimeoutMillis == null || idleTimeoutMillis <= 0) { - idleTimeoutMillis = (long) HttpClientUtil.DEFAULT_SO_TIMEOUT; - } - if (connectionTimeoutMillis == null) { - connectionTimeoutMillis = (long) HttpClientUtil.DEFAULT_CONNECT_TIMEOUT; - } return new HttpJdkSolrClient(baseSolrUrl, this); } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java index 15b7f12deba..e1056c20e01 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java @@ -57,8 +57,6 @@ public abstract class HttpSolrClientBase extends SolrClient { /** The URL of the Solr server. */ protected final String serverBaseUrl; - protected final long idleTimeoutMillis; - protected final long requestTimeoutMillis; protected final Set urlParamNames; @@ -85,11 +83,7 @@ protected HttpSolrClientBase(String serverBaseUrl, HttpSolrClientBuilderBase getUrlParamNames() { return urlParamNames; } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java index 19025c9b6ec..453f296cffc 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderBase.java @@ -25,6 +25,7 @@ public abstract class HttpSolrClientBuilderBase< B extends HttpSolrClientBuilderBase, C extends HttpSolrClientBase> { + protected Long idleTimeoutMillis; protected Long connectionTimeoutMillis; protected Long requestTimeoutMillis; @@ -115,38 +116,48 @@ public B withMaxConnectionsPerHost(int max) { return (B) this; } + /** + * The max time a connection can be idle (that is, without traffic of bytes in either direction). + * Sometimes called a "socket timeout". Note: not applicable to the JDK HttpClient. + */ @SuppressWarnings("unchecked") public B withIdleTimeout(long idleConnectionTimeout, TimeUnit unit) { this.idleTimeoutMillis = TimeUnit.MILLISECONDS.convert(idleConnectionTimeout, unit); return (B) this; } - public Long getIdleTimeoutMillis() { - return idleTimeoutMillis; + public long getIdleTimeoutMillis() { + return idleTimeoutMillis != null && idleTimeoutMillis > 0 + ? idleTimeoutMillis + : HttpClientUtil.DEFAULT_SO_TIMEOUT; } + /** The max time a connection can take to connect to destinations. */ @SuppressWarnings("unchecked") public B withConnectionTimeout(long connectionTimeout, TimeUnit unit) { this.connectionTimeoutMillis = TimeUnit.MILLISECONDS.convert(connectionTimeout, unit); return (B) this; } - public Long getConnectionTimeout() { - return connectionTimeoutMillis; + public long getConnectionTimeoutMillis() { + return connectionTimeoutMillis != null && connectionTimeoutMillis > 0 + ? connectionTimeoutMillis + : HttpClientUtil.DEFAULT_CONNECT_TIMEOUT; } - /** - * Set a timeout in milliseconds for requests issued by this client. - * - * @param requestTimeout The timeout in milliseconds - * @return this Builder. - */ + /** Set a timeout for requests to receive a response. */ @SuppressWarnings("unchecked") public B withRequestTimeout(long requestTimeout, TimeUnit unit) { this.requestTimeoutMillis = TimeUnit.MILLISECONDS.convert(requestTimeout, unit); return (B) this; } + public long getRequestTimeoutMillis() { + return requestTimeoutMillis != null && requestTimeoutMillis > 0 + ? requestTimeoutMillis + : getIdleTimeoutMillis(); + } + /** * If true, prefer http1.1 over http2. If not set, the default is determined by system property * 'solr.http1'. Otherwise, false. diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java index b9625ad5132..a90cc8c14ef 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java @@ -24,6 +24,7 @@ import java.io.InputStream; import java.lang.invoke.MethodHandles; import java.net.URISyntaxException; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -34,6 +35,7 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.HttpException; @@ -100,6 +102,24 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) } } + public static class SlowStreamServlet extends HttpServlet { + + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { + IntStream.range(0, 10) + .forEach( + i -> { + try { + Thread.sleep(500); + resp.getOutputStream().write(String.valueOf(i).getBytes(StandardCharsets.UTF_8)); + resp.getOutputStream().flush(); + } catch (IOException | InterruptedException e) { + throw new RuntimeException(e); + } + }); + } + } + public static class DebugServlet extends HttpServlet { public static void clear() { lastMethod = null; @@ -201,6 +221,7 @@ public static void beforeTest() throws Exception { .withServlet(new ServletHolder(RedirectServlet.class), "/redirect/*") .withServlet(new ServletHolder(SlowServlet.class), "/slow/*") .withServlet(new ServletHolder(DebugServlet.class), "/debug/*") + .withServlet(new ServletHolder(SlowStreamServlet.class), "/slowStream/*") .build(); createAndStartJetty(legacyExampleCollection1SolrHome(), jettyConfig); } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java index 43c04076c38..484bb38e207 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java @@ -17,10 +17,14 @@ package org.apache.solr.client.solrj.impl; +import static org.apache.solr.handler.admin.api.ReplicationAPIBase.FILE_STREAM; + import java.io.IOException; +import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.util.Base64; import java.util.Collections; +import java.util.List; import java.util.concurrent.TimeUnit; import org.apache.solr.client.api.util.SolrVersion; import org.apache.solr.client.solrj.ResponseParser; @@ -35,6 +39,7 @@ import org.apache.solr.common.params.MapSolrParams; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.common.params.SolrParams; +import org.apache.solr.common.util.NamedList; import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.util.ssl.SslContextFactory; @@ -69,7 +74,7 @@ public void testTimeout() throws Exception { client.query(q, SolrRequest.METHOD.GET); fail("No exception thrown."); } catch (SolrServerException e) { - assertTrue(e.getMessage().contains("timeout") || e.getMessage().contains("Timeout")); + assertTrue(isTimeout(e)); } } @@ -100,7 +105,7 @@ public void testRequestTimeout() throws Exception { client.query(q, SolrRequest.METHOD.GET); fail("No exception thrown."); } catch (SolrServerException e) { - assertTrue(e.getMessage().contains("timeout") || e.getMessage().contains("Timeout")); + assertTrue(isTimeout(e)); } } @@ -651,26 +656,73 @@ public void testBuilder() { } @Test - public void testIdleTimeoutWithHttpClient() { + public void testIdleTimeoutWithHttpClient() throws Exception { + String url = getBaseUrl() + SLOW_STREAM_SERVLET_PATH; try (Http2SolrClient oldClient = - new Http2SolrClient.Builder("baseSolrUrl") - .withIdleTimeout(5000, TimeUnit.MILLISECONDS) + new Http2SolrClient.Builder(url) + .withRequestTimeout(Long.MAX_VALUE, TimeUnit.MILLISECONDS) + .withIdleTimeout(100, TimeUnit.MILLISECONDS) .build()) { + try (Http2SolrClient onlyBaseUrlChangedClient = - new Http2SolrClient.Builder("newBaseSolrUrl").withHttpClient(oldClient).build()) { + new Http2SolrClient.Builder(url).withHttpClient(oldClient).build()) { assertEquals(oldClient.getIdleTimeout(), onlyBaseUrlChangedClient.getIdleTimeout()); assertEquals(oldClient.getHttpClient(), onlyBaseUrlChangedClient.getHttpClient()); } + + // too little time to succeed + QueryRequest req = new QueryRequest(); + req.setResponseParser(new InputStreamResponseParser(FILE_STREAM)); + assertExceptionThrownWithMessageContaining( + SolrServerException.class, List.of("Timeout"), () -> oldClient.request(req)); + + int newIdleTimeoutMs = 5 * 1000; // enough time to succeed try (Http2SolrClient idleTimeoutChangedClient = - new Http2SolrClient.Builder("baseSolrUrl") + new Http2SolrClient.Builder(url) .withHttpClient(oldClient) - .withIdleTimeout(3000, TimeUnit.MILLISECONDS) + .withIdleTimeout(newIdleTimeoutMs, TimeUnit.MILLISECONDS) .build()) { - assertFalse(oldClient.getIdleTimeout() == idleTimeoutChangedClient.getIdleTimeout()); - assertEquals(3000, idleTimeoutChangedClient.getIdleTimeout()); + assertNotEquals(oldClient.getIdleTimeout(), idleTimeoutChangedClient.getIdleTimeout()); + assertEquals(newIdleTimeoutMs, idleTimeoutChangedClient.getIdleTimeout()); + NamedList response = idleTimeoutChangedClient.request(req); + try (InputStream is = (InputStream) response.get("stream")) { + assertEquals("0123456789", new String(is.readAllBytes(), StandardCharsets.UTF_8)); + } } } } + @Test + public void testRequestTimeoutWithHttpClient() throws Exception { + String url = getBaseUrl() + SLOW_STREAM_SERVLET_PATH; + try (Http2SolrClient oldClient = + new Http2SolrClient.Builder(url) + .withIdleTimeout(1000, TimeUnit.MILLISECONDS) + .withRequestTimeout(10, TimeUnit.SECONDS) + .build()) { + QueryRequest req = new QueryRequest(); + req.setResponseParser(new InputStreamResponseParser(FILE_STREAM)); + int newRequestTimeoutMs = 2000; + try (Http2SolrClient requestTimeoutChangedClient = + new Http2SolrClient.Builder(url) + .withHttpClient(oldClient) + .withRequestTimeout(newRequestTimeoutMs, TimeUnit.MILLISECONDS) + .build()) { + NamedList response = requestTimeoutChangedClient.request(req); + try (InputStream is = (InputStream) response.get("stream")) { + assertExceptionThrownWithMessageContaining( + IOException.class, List.of("Total timeout"), is::readAllBytes); + } + } + NamedList response = oldClient.request(req); + try (InputStream is = (InputStream) response.get("stream")) { + assertEquals("0123456789", new String(is.readAllBytes(), StandardCharsets.UTF_8)); + } + } + } + + private static boolean isTimeout(SolrServerException e) { + return e.getMessage().contains("timeout") || e.getMessage().contains("Timeout"); + } /* Missed tests : - set cookies via interceptor - invariant params - compression */ } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java index ce6c407269d..2b8077ccc1e 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java @@ -63,6 +63,8 @@ public abstract class HttpSolrClientTestBase extends SolrJettyTestBase { protected static final String DEBUG_SERVLET_REGEX = DEBUG_SERVLET_PATH + "/*"; protected static final String REDIRECT_SERVLET_PATH = "/redirect"; protected static final String REDIRECT_SERVLET_REGEX = REDIRECT_SERVLET_PATH + "/*"; + protected static final String SLOW_STREAM_SERVLET_PATH = "/slowStream"; + protected static final String SLOW_STREAM_SERVLET_REGEX = SLOW_STREAM_SERVLET_PATH + "/*"; protected static final String COLLECTION_1 = "collection1"; // example chars that must be URI encoded - non-ASCII and curly quote protected static final String MUST_ENCODE = "\u1234\u007B"; @@ -77,6 +79,9 @@ public static void beforeTest() throws Exception { .withServlet( new ServletHolder(BasicHttpSolrClientTest.SlowServlet.class), SLOW_SERVLET_REGEX) .withServlet(new ServletHolder(DebugServlet.class), DEBUG_SERVLET_REGEX) + .withServlet( + new ServletHolder(BasicHttpSolrClientTest.SlowStreamServlet.class), + SLOW_STREAM_SERVLET_REGEX) .withSSLConfig(sslConfig.buildServerSSLConfig()) .build(); createAndStartJetty(legacyExampleCollection1SolrHome(), jettyConfig);