This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch branch_9x
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/branch_9x by this push:
new 1a7647136a6 SOLR-17776: Harmonize SolrJ timeouts (#3357)
1a7647136a6 is described below
commit 1a7647136a6e014bc003018c8e627e358cdf2d7c
Author: David Smiley <[email protected]>
AuthorDate: Wed Jun 18 20:06:29 2025 -0400
SOLR-17776: Harmonize SolrJ timeouts (#3357)
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.
The HttpJdkSolrClient wasn't setting the connection timeout as per the
builder configuration.
Co-authored-by: Luke Kot-Zaniewski <[email protected]>
(cherry picked from commit dd89cd604a6dd563f15dfcec4f7a9ce5076b2410)
---
solr/CHANGES.txt | 7 +
.../src/java/org/apache/solr/cloud/Overseer.java | 1 -
.../java/org/apache/solr/cloud/SyncStrategy.java | 1 -
.../java/org/apache/solr/handler/IndexFetcher.java | 4 -
.../pages/major-changes-in-solr-9.adoc | 8 +
.../solr/client/solrj/io/SolrClientCache.java | 16 +-
.../impl/ConcurrentUpdateHttp2SolrClient.java | 1 +
.../solr/client/solrj/impl/Http2SolrClient.java | 239 +++++++++++----------
.../solr/client/solrj/impl/HttpJdkSolrClient.java | 20 +-
.../solr/client/solrj/impl/HttpSolrClientBase.java | 17 +-
.../solrj/impl/HttpSolrClientBuilderBase.java | 31 ++-
.../client/solrj/impl/BasicHttpSolrClientTest.java | 21 ++
.../client/solrj/impl/Http2SolrClientTest.java | 72 ++++++-
.../client/solrj/impl/HttpSolrClientTestBase.java | 5 +
14 files changed, 263 insertions(+), 180 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 137f6e7bdb1..228a0ce6df8 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -61,6 +61,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 ab666a70bf4..8210a052dfb 100644
--- a/solr/core/src/java/org/apache/solr/cloud/Overseer.java
+++ b/solr/core/src/java/org/apache/solr/cloud/Overseer.java
@@ -861,7 +861,6 @@ public class Overseer implements SolrCloseable {
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 @@ public class SyncStrategy {
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 a81b570eaee..cc18d79fe7c 100644
--- a/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
+++ b/solr/core/src/java/org/apache/solr/handler/IndexFetcher.java
@@ -186,8 +186,6 @@ public class IndexFetcher {
private final Http2SolrClient solrClient;
- private Integer connTimeout;
-
private Integer soTimeout;
private boolean skipCommitOnLeaderVersionZero = true;
@@ -265,7 +263,6 @@ public class IndexFetcher {
.withHttpClient(updateShardHandler.getRecoveryOnlyHttpClient())
.withBasicAuthCredentials(httpBasicAuthUser, httpBasicAuthPassword)
.withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
- .withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS)
.build();
}
@@ -300,7 +297,6 @@ public class 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 2005d6c6100..a220a63b8f8 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
In `solr.xml`, the `indexSearcherExecutorThreads` now defaults to 0.
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 d6ee3847ca0..4cd1c627312 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
@@ -189,17 +189,13 @@ public class SolrClientCache implements Closeable {
final var builder = new Http2SolrClient.Builder(url);
if (http2SolrClient != null) {
builder.withHttpClient(http2SolrClient);
+ // cannot set connection timeout
+ } else {
+ builder.withConnectionTimeout(minConnTimeout, TimeUnit.MILLISECONDS);
}
- 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);
+ builder.withIdleTimeout(
+ Math.max(minSocketTimeout, builder.getIdleTimeoutMillis()),
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 5065256070c..eb5e6be8e4d 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
@@ -256,6 +256,7 @@ public class ConcurrentUpdateHttp2SolrClient extends
SolrClient {
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 5355c7a4b60..73d20a69d80 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
@@ -42,6 +42,7 @@ import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;
import org.apache.solr.client.api.util.SolrVersion;
import org.apache.solr.client.solrj.ResponseParser;
+import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrClientFunction;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrResponse;
@@ -98,15 +99,12 @@ import org.slf4j.LoggerFactory;
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.
*
- * <ul>
- * <li>{@link Http2SolrClient} sends requests in HTTP/2
- * <li>{@link Http2SolrClient} can point to multiple urls
- * <li>{@link Http2SolrClient} does not expose its internal httpClient like
{@link
- * HttpSolrClient#getHttpClient()}, sharing connection pools should be
done by {@link
- * Http2SolrClient.Builder#withHttpClient(Http2SolrClient)}
- * </ul>
+ * <p>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());
@@ -118,6 +116,8 @@ public class Http2SolrClient extends HttpSolrClientBase {
private final HttpClient httpClient;
+ private final long idleTimeoutMillis;
+
private List<HttpListenerFactory> listenerFactory = new ArrayList<>();
protected AsyncTracker asyncTracker = new AsyncTracker();
@@ -133,6 +133,20 @@ public class Http2SolrClient extends HttpSolrClientBase {
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;
@@ -148,8 +162,19 @@ public class Http2SolrClient extends HttpSolrClientBase {
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);
}
@@ -162,6 +187,29 @@ public class Http2SolrClient extends HttpSolrClientBase {
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);
@@ -178,8 +226,6 @@ public class Http2SolrClient extends HttpSolrClientBase {
}
private HttpClient createHttpClient(Builder builder) {
- HttpClient httpClient;
-
executor = builder.executor;
if (executor == null) {
BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(256, 256);
@@ -191,21 +237,24 @@ public class Http2SolrClient extends HttpSolrClientBase {
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");
@@ -227,6 +276,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
clientConnector.setSslContextFactory(sslContextFactory);
clientConnector.setSelectors(2);
+ HttpClient httpClient;
HttpClientTransport transport;
if (builder.useHttp1_1) {
if (log.isDebugEnabled()) {
@@ -256,17 +306,17 @@ public class Http2SolrClient extends HttpSolrClientBase {
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.setCookieStore(builder.cookieStore);
+ var cookieStore = builder.getCookieStore();
+ if (cookieStore != null) {
+ httpClient.setCookieStore(cookieStore);
}
this.authenticationStore = new AuthenticationStoreHolder();
httpClient.setAuthenticationStore(this.authenticationStore);
- httpClient.setConnectTimeout(builder.connectionTimeoutMillis);
-
setupProxy(builder, httpClient);
try {
@@ -289,15 +339,14 @@ public class Http2SolrClient extends HttpSolrClientBase {
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);
}
@@ -333,6 +382,11 @@ public class Http2SolrClient extends HttpSolrClientBase {
this.authenticationStore.updateAuthenticationStore(authenticationStore);
}
+ /** (visible for testing) */
+ public long getIdleTimeout() {
+ return idleTimeoutMillis;
+ }
+
public static class OutStream implements Closeable {
private final String origCollection;
private final ModifiableSolrParams origParams;
@@ -536,6 +590,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
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();
@@ -611,13 +666,18 @@ public class Http2SolrClient extends HttpSolrClientBase {
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(),
@@ -643,12 +703,9 @@ public class Http2SolrClient extends HttpSolrClientBase {
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());
}
@@ -724,22 +781,26 @@ public class Http2SolrClient extends HttpSolrClientBase {
if (SolrRequest.METHOD.POST == solrRequest.getMethod()
|| SolrRequest.METHOD.PUT == solrRequest.getMethod()) {
- RequestWriter.ContentWriter contentWriter =
requestWriter.getContentWriter(solrRequest);
- Collection<ContentStream> 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<ContentStream> 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));
@@ -796,14 +857,18 @@ public class Http2SolrClient extends HttpSolrClientBase {
}
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.addFilePart(
@@ -817,6 +882,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
}
} 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;
@@ -1083,7 +1149,10 @@ public class Http2SolrClient extends HttpSolrClientBase {
return this;
}
- private CookieStore getDefaultCookieStore() {
+ private CookieStore getCookieStore() {
+ if (cookieStore == null) {
+ return cookieStore;
+ }
if (Boolean.getBoolean("solr.http.disableCookies")) {
return new HttpCookieStore.Empty();
}
@@ -1100,65 +1169,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
@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);
}
/**
@@ -1171,18 +1182,15 @@ public class Http2SolrClient extends HttpSolrClientBase
{
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;
}
@@ -1193,8 +1201,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
this.defaultCollection = http2SolrClient.defaultCollection;
}
if (this.listenerFactory == null) {
- this.listenerFactory = new ArrayList<HttpListenerFactory>();
- 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 c0b3ce90e1a..8d5858bab2e 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
@@ -88,12 +88,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);
}
@@ -429,11 +435,7 @@ public class HttpJdkSolrClient extends HttpSolrClientBase {
}
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<String, String> headers = solrRequest.getHeaders();
@@ -580,12 +582,6 @@ public class HttpJdkSolrClient extends HttpSolrClientBase {
@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 6bc1cd17f62..b29195f3d9e 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
@@ -59,8 +59,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<String> urlParamNames;
@@ -87,11 +85,7 @@ public abstract class HttpSolrClientBase extends SolrClient {
} else {
this.serverBaseUrl = null;
}
- if (builder.idleTimeoutMillis != null) {
- this.idleTimeoutMillis = builder.idleTimeoutMillis;
- } else {
- this.idleTimeoutMillis = -1;
- }
+ this.requestTimeoutMillis = builder.getRequestTimeoutMillis();
this.basicAuthAuthorizationStr = builder.basicAuthAuthorizationStr;
if (builder.requestWriter != null) {
this.requestWriter = builder.requestWriter;
@@ -100,11 +94,6 @@ public abstract class HttpSolrClientBase extends SolrClient
{
this.parser = builder.responseParser;
}
this.defaultCollection = builder.defaultCollection;
- if (builder.requestTimeoutMillis != null) {
- this.requestTimeoutMillis = builder.requestTimeoutMillis;
- } else {
- this.requestTimeoutMillis = -1;
- }
if (builder.urlParamNames != null) {
this.urlParamNames = builder.urlParamNames;
} else {
@@ -411,10 +400,6 @@ public abstract class HttpSolrClientBase extends
SolrClient {
return parser;
}
- public long getIdleTimeout() {
- return idleTimeoutMillis;
- }
-
public Set<String> 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 @@ import org.apache.solr.client.solrj.request.RequestWriter;
public abstract class HttpSolrClientBuilderBase<
B extends HttpSolrClientBuilderBase<?, ?>, C extends HttpSolrClientBase> {
+
protected Long idleTimeoutMillis;
protected Long connectionTimeoutMillis;
protected Long requestTimeoutMillis;
@@ -115,38 +116,48 @@ public abstract class HttpSolrClientBuilderBase<
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 0e36df1bbd5..6a8456310f2 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
@@ -20,6 +20,7 @@ import java.io.IOException;
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;
@@ -30,6 +31,7 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
+import java.util.stream.IntStream;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
@@ -100,6 +102,24 @@ public class BasicHttpSolrClientTest extends
SolrJettyTestBase {
}
}
+ 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 class BasicHttpSolrClientTest extends
SolrJettyTestBase {
.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 24825bf33c3..8453269c2aa 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,11 +17,15 @@
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.HashMap;
+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.SolrException;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.MapSolrParams;
import org.apache.solr.common.params.ModifiableSolrParams;
+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 class Http2SolrClientTest extends
HttpSolrClientTestBase {
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 class Http2SolrClientTest extends
HttpSolrClientTestBase {
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));
}
}
@@ -687,26 +692,73 @@ public class Http2SolrClientTest extends
HttpSolrClientTestBase {
}
@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<Object> 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<Object> response = requestTimeoutChangedClient.request(req);
+ try (InputStream is = (InputStream) response.get("stream")) {
+ assertExceptionThrownWithMessageContaining(
+ IOException.class, List.of("Total timeout"), is::readAllBytes);
+ }
+ }
+ NamedList<Object> 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 4e43356b7dd..3bcbd9dc0ae 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 abstract class HttpSolrClientTestBase extends
SolrJettyTestBase {
.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);
