Avoid setting connection request timeout (#30384)

We've been setting this value to 500ms in the default low-level REST client configuration, misunderstanding the effect that it would have. This proved very problematic, as it ends up causing `TimeoutException` returned from the leased pool in some cases even for successful requests. Closes #24069

Author: javanna

client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java

@@ -43,7 +43,6 @@ public final class RestClientBuilder {
  public static final int DEFAULT_CONNECT_TIMEOUT_MILLIS = 1000;
  public static final int DEFAULT_SOCKET_TIMEOUT_MILLIS = 30000;
  public static final int DEFAULT_MAX_RETRY_TIMEOUT_MILLIS = DEFAULT_SOCKET_TIMEOUT_MILLIS;
- public static final int DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS = 500;
  public static final int DEFAULT_MAX_CONN_PER_ROUTE = 10;
  public static final int DEFAULT_MAX_CONN_TOTAL = 30;
 
@@ -196,8 +195,7 @@ private CloseableHttpAsyncClient createHttpClient() {
  //default timeouts are all infinite
  RequestConfig.Builder requestConfigBuilder = RequestConfig.custom()
  .setConnectTimeout(DEFAULT_CONNECT_TIMEOUT_MILLIS)
- .setSocketTimeout(DEFAULT_SOCKET_TIMEOUT_MILLIS)
- .setConnectionRequestTimeout(DEFAULT_CONNECTION_REQUEST_TIMEOUT_MILLIS);
+ .setSocketTimeout(DEFAULT_SOCKET_TIMEOUT_MILLIS);
  if (requestConfigCallback != null) {
  requestConfigBuilder = requestConfigCallback.customizeRequestConfig(requestConfigBuilder);
  }

client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java

@@ -177,4 +177,24 @@ private static void assertSetPathPrefixThrows(final String pathPrefix) {
  }
  }
 
+ /**
+ * This test verifies that we don't change the default value for the connection request timeout as that causes problems.
+ * See https://github.com/elastic/elasticsearch/issues/24069
+ */
+ public void testDefaultConnectionRequestTimeout() throws IOException {
+ RestClientBuilder builder = RestClient.builder(new HttpHost("localhost", 9200));
+ builder.setRequestConfigCallback(new RestClientBuilder.RequestConfigCallback() {
+ @Override
+ public RequestConfig.Builder customizeRequestConfig(RequestConfig.Builder requestConfigBuilder) {
+ RequestConfig requestConfig = requestConfigBuilder.build();
+ assertEquals(RequestConfig.DEFAULT.getConnectionRequestTimeout(), requestConfig.getConnectionRequestTimeout());
+ //this way we get notified if the default ever changes
+ assertEquals(-1, requestConfig.getConnectionRequestTimeout());
+ return requestConfigBuilder;
+ }
+ });
+ try (RestClient restClient = builder.build()) {
+ assertNotNull(restClient);
+ }
+ }
 }

client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java

@@ -32,6 +32,7 @@
 import org.apache.http.entity.StringEntity;
 import org.apache.http.impl.client.BasicCredentialsProvider;
 import org.apache.http.impl.nio.client.HttpAsyncClientBuilder;
+import org.apache.http.nio.entity.NStringEntity;
 import org.apache.http.util.EntityUtils;
 import org.elasticsearch.mocksocket.MockHttpServer;
 import org.junit.AfterClass;
@@ -48,6 +49,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 
 import static org.elasticsearch.client.RestClientTestUtil.getAllStatusCodes;
 import static org.elasticsearch.client.RestClientTestUtil.getHttpMethods;
@@ -159,6 +163,42 @@ public static void stopHttpServers() throws IOException {
  httpServer = null;
  }
 
+ /**
+ * Tests sending a bunch of async requests works well (e.g. no TimeoutException from the leased pool)
+ * See https://github.com/elastic/elasticsearch/issues/24069
+ */
+ public void testManyAsyncRequests() throws Exception {
+ int iters = randomIntBetween(500, 1000);
+ final CountDownLatch latch = new CountDownLatch(iters);
+ final List<Exception> exceptions = new CopyOnWriteArrayList<>();
+ for (int i = 0; i < iters; i++) {
+ Request request = new Request("PUT", "/200");
+ request.setEntity(new NStringEntity("{}", ContentType.APPLICATION_JSON));
+ restClient.performRequestAsync(request, new ResponseListener() {
+ @Override
+ public void onSuccess(Response response) {
+ latch.countDown();
+ }
+
+ @Override
+ public void onFailure(Exception exception) {
+ exceptions.add(exception);
+ latch.countDown();
+ }
+ });
+ }
+
+ assertTrue("timeout waiting for requests to be sent", latch.await(10, TimeUnit.SECONDS));
+ if (exceptions.isEmpty() == false) {
+ AssertionError error = new AssertionError("expected no failures but got some. see suppressed for first 10 of ["
+ + exceptions.size() + "] failures");
+ for (Exception exception : exceptions.subList(0, Math.min(10, exceptions.size()))) {
+ error.addSuppressed(exception);
+ }
+ throw error;
+ }
+ }
+
  /**
  * End to end test for headers. We test it explicitly against a real http client as there are different ways
  * to set/add headers to the {@link org.apache.http.client.HttpClient}.