This is an automated email from the ASF dual-hosted git repository.
epugh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new 44cb2b4436e SOLR-13605: HttpSolrClient.Builder.withHttpClient() is
useless for the purpose of setting client scoped so/connect timeouts (#1565)
44cb2b4436e is described below
commit 44cb2b4436edd23acb31a0e862901ae567b0186c
Author: Eric Pugh <[email protected]>
AuthorDate: Sat Apr 29 10:31:57 2023 -0400
SOLR-13605: HttpSolrClient.Builder.withHttpClient() is useless for the
purpose of setting client scoped so/connect timeouts (#1565)
Properly handling HttpClient properties.
---------
Co-authored-by: Alex Deparvu <[email protected]>
---
solr/CHANGES.txt | 4 ++-
.../client/solrj/impl/CloudLegacySolrClient.java | 21 +++---------
.../solrj/impl/ConcurrentUpdateSolrClient.java | 16 ++++-----
.../solr/client/solrj/impl/HttpSolrClient.java | 40 ++++++++++++++++------
.../solr/client/solrj/impl/LBHttpSolrClient.java | 22 ++++--------
.../solr/client/solrj/impl/SolrClientBuilder.java | 36 +++++++++++++------
.../solrj/impl/HttpSolrClientBuilderTest.java | 17 +++++++++
.../solrj/impl/LBHttpSolrClientBuilderTest.java | 19 ++++++++++
8 files changed, 112 insertions(+), 63 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 2891ada69bb..8ed03ae6193 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -88,7 +88,7 @@ Improvements
`GET /api/node/logging/messages`. And the threshold for the log "listener"
can be modified with
`PUT /api/node/logging/messages/threshold`. (Jason Gerlowski, Calvince
Otieno)
-* SOLR-16504: Convert CLI tools to use Jetty HTTP 2 client. (Bence Szabo via
Eric Pugh)
+* SOLR-16504: Convert CLI tools to use Jetty HTTP 2 client. (Bence Szabo via
Eric Pugh)
* SOLR-15737: Solr's collection-level "snapshot" APIs now have v2 equivalents.
Snapshots can be created at `POST
/api/collections/collName/snapshots/snapshotName`, listed at `GET
/api/collections/collName/snapshots`, and deleted at
@@ -162,6 +162,8 @@ Bug Fixes
* SOLR-16737: Http2SolrClient needs to inherit all properties when initialized
with another http2 client (Alex Deparvu, Tomás Fernández Löbbe)
+* SOLR-13605: Fix setting client scoped socket and connect timeouts when using
HttpSolrClient.Builder.withHttpClient() method. (Eric Pugh, Alex Deparvu)
+
Dependency Upgrades
---------------------
* PR#1494: Upgrade forbiddenapis to 3.5 (Uwe Schindler)
diff --git
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudLegacySolrClient.java
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudLegacySolrClient.java
index 3299236dd44..4a94359fca5 100644
---
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudLegacySolrClient.java
+++
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudLegacySolrClient.java
@@ -107,14 +107,8 @@ public class CloudLegacySolrClient extends CloudSolrClient
{
private void propagateLBClientConfigOptions(Builder builder) {
final LBHttpSolrClient.Builder lbBuilder = builder.lbClientBuilder;
-
- if (builder.connectionTimeoutMillis != null) {
- lbBuilder.withConnectionTimeout(builder.connectionTimeoutMillis,
TimeUnit.MILLISECONDS);
- }
-
- if (builder.socketTimeoutMillis != null) {
- lbBuilder.withSocketTimeout(builder.socketTimeoutMillis,
TimeUnit.MILLISECONDS);
- }
+ lbBuilder.withConnectionTimeout(builder.connectionTimeoutMillis,
TimeUnit.MILLISECONDS);
+ lbBuilder.withSocketTimeout(builder.socketTimeoutMillis,
TimeUnit.MILLISECONDS);
}
@Override
@@ -177,14 +171,9 @@ public class CloudLegacySolrClient extends CloudSolrClient
{
Builder cloudSolrClientBuilder, HttpClient httpClient) {
final LBHttpSolrClient.Builder lbBuilder = new LBHttpSolrClient.Builder();
lbBuilder.withHttpClient(httpClient);
- if (cloudSolrClientBuilder.connectionTimeoutMillis != null) {
- lbBuilder.withConnectionTimeout(
- cloudSolrClientBuilder.connectionTimeoutMillis,
TimeUnit.MILLISECONDS);
- }
- if (cloudSolrClientBuilder.socketTimeoutMillis != null) {
- lbBuilder.withSocketTimeout(
- cloudSolrClientBuilder.socketTimeoutMillis, TimeUnit.MILLISECONDS);
- }
+ lbBuilder.withConnectionTimeout(
+ cloudSolrClientBuilder.connectionTimeoutMillis, TimeUnit.MILLISECONDS);
+ lbBuilder.withSocketTimeout(cloudSolrClientBuilder.socketTimeoutMillis,
TimeUnit.MILLISECONDS);
lbBuilder.withRequestWriter(new BinaryRequestWriter());
lbBuilder.withResponseParser(new BinaryResponseParser());
diff --git
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java
index cc0f9e3e6f2..3141714f21d 100644
---
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java
+++
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateSolrClient.java
@@ -85,8 +85,8 @@ public class ConcurrentUpdateSolrClient extends SolrClient {
int stallTimeMillis;
private final boolean streamDeletes;
private boolean internalHttpClient;
- private volatile Integer connectionTimeout;
- private volatile Integer soTimeout;
+ private final int connectionTimeout;
+ private final int soTimeout;
private volatile boolean closed;
AtomicInteger pollInterrupts;
@@ -112,8 +112,8 @@ public class ConcurrentUpdateSolrClient extends SolrClient {
this.threadCount = builder.threadCount;
this.runners = new ArrayDeque<>();
this.streamDeletes = builder.streamDeletes;
- this.connectionTimeout = Math.toIntExact(builder.connectionTimeoutMillis);
- this.soTimeout = Math.toIntExact(builder.socketTimeoutMillis);
+ this.connectionTimeout = builder.connectionTimeoutMillis;
+ this.soTimeout = builder.socketTimeoutMillis;
this.pollQueueTimeMillis = builder.pollQueueTime;
this.stallTimeMillis = Integer.getInteger("solr.cloud.client.stallTime",
15000);
@@ -322,12 +322,8 @@ public class ConcurrentUpdateSolrClient extends SolrClient
{
org.apache.http.client.config.RequestConfig.Builder
requestConfigBuilder =
HttpClientUtil.createDefaultRequestConfigBuilder();
- if (soTimeout != null) {
- requestConfigBuilder.setSocketTimeout(soTimeout);
- }
- if (connectionTimeout != null) {
- requestConfigBuilder.setConnectTimeout(connectionTimeout);
- }
+ requestConfigBuilder.setSocketTimeout(soTimeout);
+ requestConfigBuilder.setConnectTimeout(connectionTimeout);
method.setConfig(requestConfigBuilder.build());
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 0e0b81f3dc9..5f7b8a17ee8 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
@@ -149,8 +149,8 @@ public class HttpSolrClient extends BaseHttpSolrClient {
private final boolean internalClient;
private volatile Set<String> urlParamNames = Set.of();
- private volatile Integer connectionTimeout;
- private volatile Integer soTimeout;
+ private final int connectionTimeout;
+ private final int soTimeout;
/** Use the builder to create this client */
protected HttpSolrClient(Builder builder) {
@@ -168,13 +168,14 @@ public class HttpSolrClient extends BaseHttpSolrClient {
this.internalClient = false;
this.followRedirects = builder.followRedirects;
this.httpClient = builder.httpClient;
-
} else {
this.internalClient = true;
this.followRedirects = builder.followRedirects;
ModifiableSolrParams params = new ModifiableSolrParams();
params.set(HttpClientUtil.PROP_FOLLOW_REDIRECTS, followRedirects);
params.set(HttpClientUtil.PROP_ALLOW_COMPRESSION, builder.compression);
+ params.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT,
builder.connectionTimeoutMillis);
+ params.set(HttpClientUtil.PROP_SO_TIMEOUT, builder.socketTimeoutMillis);
httpClient = HttpClientUtil.createClient(params);
}
@@ -184,8 +185,8 @@ public class HttpSolrClient extends BaseHttpSolrClient {
this.parser = builder.responseParser;
this.invariantParams = builder.invariantParams;
- this.connectionTimeout = Math.toIntExact(builder.connectionTimeoutMillis);
- this.soTimeout = Math.toIntExact(builder.socketTimeoutMillis);
+ this.connectionTimeout = builder.connectionTimeoutMillis;
+ this.soTimeout = builder.socketTimeoutMillis;
this.useMultiPartPost = builder.useMultiPartPost;
this.urlParamNames = builder.urlParamNames;
}
@@ -194,6 +195,26 @@ public class HttpSolrClient extends BaseHttpSolrClient {
return urlParamNames;
}
+ /**
+ * Returns the connection timeout, and should be based on httpClient
overriding the solrClient.
+ * For unit testing.
+ *
+ * @return the connection timeout
+ */
+ int getConnectionTimeout() {
+ return this.connectionTimeout;
+ }
+
+ /**
+ * Returns the socket timeout, and should be based on httpClient overriding
the solrClient. For
+ * unit testing.
+ *
+ * @return the socket timeout
+ */
+ int getSocketTimeout() {
+ return this.soTimeout;
+ }
+
/**
* Process the request. If {@link
org.apache.solr.client.solrj.SolrRequest#getResponseParser()} is
* null, then use {@link #getParser()}
@@ -548,12 +569,9 @@ public class HttpSolrClient extends BaseHttpSolrClient {
org.apache.http.client.config.RequestConfig.Builder requestConfigBuilder =
HttpClientUtil.createDefaultRequestConfigBuilder();
- if (soTimeout != null) {
- requestConfigBuilder.setSocketTimeout(soTimeout);
- }
- if (connectionTimeout != null) {
- requestConfigBuilder.setConnectTimeout(connectionTimeout);
- }
+ requestConfigBuilder.setSocketTimeout(soTimeout);
+ requestConfigBuilder.setConnectTimeout(connectionTimeout);
+
if (followRedirects != null) {
requestConfigBuilder.setRedirectsEnabled(followRedirects);
}
diff --git
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java
index f1e6d6ace0e..c6ca02f6753 100644
---
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java
+++
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBHttpSolrClient.java
@@ -81,8 +81,8 @@ public class LBHttpSolrClient extends LBSolrClient {
private final HttpSolrClient.Builder httpSolrClientBuilder;
private volatile Set<String> urlParamNames = new HashSet<>();
- private Long connectionTimeoutMillis;
- private Long soTimeoutMillis;
+ final int connectionTimeoutMillis;
+ final int soTimeoutMillis;
/** The provided httpClient should use a multi-threaded connection manager */
protected LBHttpSolrClient(Builder builder) {
@@ -118,13 +118,9 @@ public class LBHttpSolrClient extends LBSolrClient {
if (httpSolrClientBuilder != null) {
synchronized (this) {
httpSolrClientBuilder.withBaseSolrUrl(server).withHttpClient(httpClient);
- if (connectionTimeoutMillis != null) {
- httpSolrClientBuilder.withConnectionTimeout(
- connectionTimeoutMillis, TimeUnit.MILLISECONDS);
- }
- if (soTimeoutMillis != null) {
- httpSolrClientBuilder.withSocketTimeout(soTimeoutMillis,
TimeUnit.MILLISECONDS);
- }
+ httpSolrClientBuilder.withConnectionTimeout(connectionTimeoutMillis,
TimeUnit.MILLISECONDS);
+ httpSolrClientBuilder.withSocketTimeout(soTimeoutMillis,
TimeUnit.MILLISECONDS);
+
if (requestWriter != null) {
httpSolrClientBuilder.withRequestWriter(requestWriter);
}
@@ -136,12 +132,8 @@ public class LBHttpSolrClient extends LBSolrClient {
} else {
final HttpSolrClient.Builder clientBuilder =
new
HttpSolrClient.Builder(server).withHttpClient(httpClient).withResponseParser(parser);
- if (connectionTimeoutMillis != null) {
- clientBuilder.withConnectionTimeout(connectionTimeoutMillis,
TimeUnit.MILLISECONDS);
- }
- if (soTimeoutMillis != null) {
- clientBuilder.withSocketTimeout(soTimeoutMillis,
TimeUnit.MILLISECONDS);
- }
+ clientBuilder.withConnectionTimeout(connectionTimeoutMillis,
TimeUnit.MILLISECONDS);
+ clientBuilder.withSocketTimeout(soTimeoutMillis, TimeUnit.MILLISECONDS);
if (requestWriter != null) {
clientBuilder.withRequestWriter(requestWriter);
}
diff --git
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java
index d6e31e5ef97..645a19040de 100644
---
a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java
+++
b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/SolrClientBuilder.java
@@ -19,6 +19,8 @@ package org.apache.solr.client.solrj.impl;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import org.apache.http.client.HttpClient;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.Configurable;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.impl.HttpSolrClient.Builder;
import org.apache.solr.client.solrj.request.RequestWriter;
@@ -34,8 +36,10 @@ public abstract class SolrClientBuilder<B extends
SolrClientBuilder<B>> {
protected ResponseParser responseParser;
protected RequestWriter requestWriter;
protected boolean useMultiPartPost;
- protected Long connectionTimeoutMillis = TimeUnit.MILLISECONDS.convert(15,
TimeUnit.SECONDS);
- protected Long socketTimeoutMillis = TimeUnit.MILLISECONDS.convert(120,
TimeUnit.SECONDS);
+ protected int connectionTimeoutMillis = 15000; // 15 seconds
+ private boolean connectionTimeoutMillisUpdate = false;
+ protected int socketTimeoutMillis = 120000; // 120 seconds
+ private boolean socketTimeoutMillisUpdate = false;
protected boolean followRedirects = false;
protected Set<String> urlParamNames;
@@ -45,6 +49,17 @@ public abstract class SolrClientBuilder<B extends
SolrClientBuilder<B>> {
/** Provides a {@link HttpClient} for the builder to use when creating
clients. */
public B withHttpClient(HttpClient httpClient) {
this.httpClient = httpClient;
+
+ if (this.httpClient instanceof Configurable) {
+ RequestConfig conf = ((Configurable) httpClient).getConfig();
+ // only update values that were not already manually changed
+ if (!connectionTimeoutMillisUpdate && conf.getConnectTimeout() > 0) {
+ this.connectionTimeoutMillis = conf.getConnectTimeout();
+ }
+ if (!socketTimeoutMillisUpdate && conf.getSocketTimeout() > 0) {
+ this.socketTimeoutMillis = conf.getSocketTimeout();
+ }
+ }
return getThis();
}
@@ -88,7 +103,7 @@ public abstract class SolrClientBuilder<B extends
SolrClientBuilder<B>> {
*
* <p>For valid values see {@link
org.apache.http.client.config.RequestConfig#getConnectTimeout()}
*
- * @deprecated Please use {@link #withConnectionTimeout(long, TimeUnit)}
+ * @deprecated Please use {@link #withConnectionTimeout(int, TimeUnit)}
*/
@Deprecated(since = "9.2")
public B withConnectionTimeout(int connectionTimeoutMillis) {
@@ -102,12 +117,13 @@ public abstract class SolrClientBuilder<B extends
SolrClientBuilder<B>> {
*
* <p>For valid values see {@link
org.apache.http.client.config.RequestConfig#getConnectTimeout()}
*/
- public B withConnectionTimeout(long connectionTimeout, TimeUnit unit) {
+ public B withConnectionTimeout(int connectionTimeout, TimeUnit unit) {
if (connectionTimeout < 0) {
throw new IllegalArgumentException("connectionTimeout must be a
non-negative integer.");
}
-
- this.connectionTimeoutMillis =
TimeUnit.MILLISECONDS.convert(connectionTimeout, unit);
+ this.connectionTimeoutMillis =
+ Math.toIntExact(TimeUnit.MILLISECONDS.convert(connectionTimeout,
unit));
+ connectionTimeoutMillisUpdate = true;
return getThis();
}
@@ -117,7 +133,7 @@ public abstract class SolrClientBuilder<B extends
SolrClientBuilder<B>> {
*
* <p>For valid values see {@link
org.apache.http.client.config.RequestConfig#getSocketTimeout()}
*
- * <p>* @deprecated Please use {@link #withSocketTimeout(long, TimeUnit)}
+ * <p>* @deprecated Please use {@link #withSocketTimeout(int, TimeUnit)}
*/
@Deprecated(since = "9.2")
public B withSocketTimeout(int socketTimeoutMillis) {
@@ -131,12 +147,12 @@ public abstract class SolrClientBuilder<B extends
SolrClientBuilder<B>> {
*
* <p>For valid values see {@link
org.apache.http.client.config.RequestConfig#getSocketTimeout()}
*/
- public B withSocketTimeout(long socketTimeout, TimeUnit unit) {
+ public B withSocketTimeout(int socketTimeout, TimeUnit unit) {
if (socketTimeout < 0) {
throw new IllegalArgumentException("socketTimeout must be a non-negative
integer.");
}
-
- this.socketTimeoutMillis = TimeUnit.MILLISECONDS.convert(socketTimeout,
unit);
+ this.socketTimeoutMillis =
Math.toIntExact(TimeUnit.MILLISECONDS.convert(socketTimeout, unit));
+ socketTimeoutMillisUpdate = true;
return getThis();
}
}
diff --git
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java
index dbe01b0be15..eea4fcfc3b8 100644
---
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java
+++
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java
@@ -23,6 +23,7 @@ import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.impl.HttpSolrClient.Builder;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
/** Unit tests for {@link Builder}. */
@@ -51,6 +52,22 @@ public class HttpSolrClientBuilderTest extends SolrTestCase {
}
}
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+ try (HttpSolrClient createdClient =
+ new Builder(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build()) {
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.getConnectionTimeout());
+ assertEquals(12345, createdClient.getSocketTimeout());
+ }
+ HttpClientUtil.close(httpClient);
+ }
+
@Test
public void testProvidesResponseParserToClient() throws IOException {
try (HttpSolrClient createdClient =
diff --git
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientBuilderTest.java
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientBuilderTest.java
index e1c0fb55008..25a70f5460c 100644
---
a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientBuilderTest.java
+++
b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttpSolrClientBuilderTest.java
@@ -17,11 +17,13 @@
package org.apache.solr.client.solrj.impl;
+import java.io.IOException;
import org.apache.http.client.HttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.solr.SolrTestCase;
import org.apache.solr.client.solrj.ResponseParser;
import org.apache.solr.client.solrj.impl.LBHttpSolrClient.Builder;
+import org.apache.solr.common.params.ModifiableSolrParams;
import org.junit.Test;
/** Unit tests for {@link Builder}. */
@@ -58,4 +60,21 @@ public class LBHttpSolrClientBuilderTest extends
SolrTestCase {
assertTrue(usedParser instanceof BinaryResponseParser);
}
}
+
+ @Test
+ public void testUsesTimeoutProvidedByHttpClient() throws IOException {
+
+ ModifiableSolrParams clientParams = new ModifiableSolrParams();
+ clientParams.set(HttpClientUtil.PROP_SO_TIMEOUT, 12345);
+ clientParams.set(HttpClientUtil.PROP_CONNECTION_TIMEOUT, 67890);
+ HttpClient httpClient = HttpClientUtil.createClient(clientParams);
+
+ try (LBHttpSolrClient createdClient =
+ new
Builder().withBaseSolrUrl(ANY_BASE_SOLR_URL).withHttpClient(httpClient).build())
{
+ assertEquals(createdClient.getHttpClient(), httpClient);
+ assertEquals(67890, createdClient.connectionTimeoutMillis);
+ assertEquals(12345, createdClient.soTimeoutMillis);
+ }
+ HttpClientUtil.close(httpClient);
+ }
}
