SOLR-17776: Harmonize SolrJ timeouts

solr/core/src/java/org/apache/solr/cloud/Overseer.java

@@ -862,7 +862,6 @@ private void doCompatCheck(BiConsumer<String, Object> consumer) {
             new Http2SolrClient.Builder()
                 .withHttpClient(getCoreContainer().getDefaultHttpSolrClient())
                 .withIdleTimeout(30000, TimeUnit.MILLISECONDS)
-                .withConnectionTimeout(15000, TimeUnit.MILLISECONDS)
                 .build();
         var client =
             new CloudHttp2SolrClient.Builder(

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);

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);

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;
   }
 

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();

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.
  *
- * <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());
@@ -114,6 +111,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();
 
@@ -144,8 +143,8 @@ protected Http2SolrClient(String serverBaseUrl, Builder builder) {
       this.listenerFactory.addAll(builder.listenerFactory);
     }
     updateDefaultMimeTypeForParser();
-
     this.httpClient.setFollowRedirects(Boolean.TRUE.equals(builder.followRedirects));
+    this.idleTimeoutMillis = builder.getIdleTimeoutMillis();
 
     assert ObjectReleaseTracker.track(this);
   }
@@ -252,7 +251,8 @@ 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);
@@ -261,8 +261,6 @@ private HttpClient createHttpClient(Builder builder) {
     this.authenticationStore = new AuthenticationStoreHolder();
     httpClient.setAuthenticationStore(this.authenticationStore);
 
-    httpClient.setConnectTimeout(builder.connectionTimeoutMillis);
-
     setupProxy(builder, httpClient);
 
     try {
@@ -329,6 +327,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 +517,7 @@ public NamedList<Object> 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();
@@ -621,12 +625,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());
     }
@@ -1016,28 +1017,31 @@ protected <B extends HttpSolrClientBase> B build(Class<B> 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);
+      if (httpClient == null) {
+        // set defaults for building an httpClient...
+        if (sslConfig == null) {
+          sslConfig = Http2SolrClient.defaultSSLConfig;
+        }
+        if (cookieStore == null) {
+          cookieStore = getDefaultCookieStore();
+        }
+        if (keyStoreReloadIntervalSecs == null
+            && Boolean.getBoolean("solr.keyStoreReload.enabled")) {
+          keyStoreReloadIntervalSecs =
+              Long.getLong("solr.jetty.sslContext.reload.scanInterval", 30);
+        }
+      } else {
+        if (followRedirects != null
+            || connectionTimeoutMillis != null
+            || maxConnectionsPerHost != null
+            || useHttp1_1 != httpClient.getTransport() instanceof HttpClientTransportOverHTTP
+            || proxyHost != null
+            || sslConfig != null
+            || cookieStore != null
+            || keyStoreReloadIntervalSecs != null) {
+          throw new IllegalArgumentException(
+              "You cannot provide the HttpClient and also specify options that are used to build a new client");
+        }
       }
 
       Http2SolrClient client = new Http2SolrClient(baseSolrUrl, this);
@@ -1087,27 +1091,23 @@ 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;
       }
       if (this.urlParamNames == null) {
         this.urlParamNames = http2SolrClient.urlParamNames;
       }
       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;

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<String, String> 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);
     }
 

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<String> urlParamNames;
@@ -85,11 +83,7 @@ protected HttpSolrClientBase(String serverBaseUrl, HttpSolrClientBuilderBase<?,
     } 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;
@@ -98,11 +92,6 @@ protected HttpSolrClientBase(String serverBaseUrl, HttpSolrClientBuilderBase<?,
       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 {
@@ -396,10 +385,6 @@ public ResponseParser getParser() {
     return parser;
   }
 
-  public long getIdleTimeout() {
-    return idleTimeoutMillis;
-  }
-
   public Set<String> getUrlParamNames() {
     return urlParamNames;
   }