From 6a86238ca718b9b45a0dd6555a8aa527cff1c9ac Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 4 May 2018 12:34:38 +0200 Subject: [PATCH 1/7] Avoid setting connection request timeout 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 --- .../client/RestClientBuilder.java | 4 +- .../client/RestClientBuilderTests.java | 17 ++++++++ .../RestClientSingleHostIntegTests.java | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java b/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java index 286ed7dd53910..8768c07161989 100644 --- a/client/rest/src/main/java/org/elasticsearch/client/RestClientBuilder.java +++ b/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); } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java index c9243d3aaf6ce..dda08cd6d8c2a 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java @@ -177,4 +177,21 @@ 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() { + 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()); + assertEquals(-1, requestConfig.getConnectionRequestTimeout()); + return null; + } + }); + builder.build(); + } } diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java index dd23dbe454fa4..3f57e8c391215 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostIntegTests.java +++ b/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 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}. From 58d375ab969aa97676fdaae9a1e7cbd1bc2ab00a Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 4 May 2018 13:47:31 +0200 Subject: [PATCH 2/7] update changelog --- docs/CHANGELOG.asciidoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 9e33c10a56b12..4bd47d874526c 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -199,6 +199,8 @@ coming[6.3.1] Reduce the number of object allocations made by {security} when resolving the indices and aliases for a request ({pull}30180[#30180]) +Stop setting connection request timeout in default low-level REST client configuration ({pull}30384[#30384]) + //[float] //=== Regressions From 6fb317925430d45dc5c5ed081ce6d4021d7b9f09 Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 4 May 2018 13:51:02 +0200 Subject: [PATCH 3/7] fix failing test --- .../org/elasticsearch/client/RestClientBuilderTests.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java index dda08cd6d8c2a..9657e782bda04 100644 --- a/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java +++ b/client/rest/src/test/java/org/elasticsearch/client/RestClientBuilderTests.java @@ -181,17 +181,20 @@ 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() { + 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 null; + return requestConfigBuilder; } }); - builder.build(); + try (RestClient restClient = builder.build()) { + assertNotNull(restClient); + } } } From 8fc20919ff2ce31819f6e12a290244ccde30adf9 Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 8 May 2018 13:51:49 +0200 Subject: [PATCH 4/7] update changelog --- docs/CHANGELOG.asciidoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 5e592de6d46bf..e0be70d1e5b7c 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -221,9 +221,11 @@ coming[6.3.1] Reduce the number of object allocations made by {security} when resolving the indices and aliases for a request ({pull}30180[#30180]) -Stop setting connection request timeout in default low-level REST client configuration ({pull}30384[#30384]) Respect accept header on requests with no handler ({pull}30383[#30383]) +Stop setting connection request timeout in default low-level REST client configuration because it +causes unexpected timeout exceptions for successful requests ({pull}30384[#30384]) + //[float] //=== Regressions From 95eb66799443b622fac99543932e53d577cb3088 Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 8 May 2018 14:28:00 +0200 Subject: [PATCH 5/7] update changelog --- docs/CHANGELOG.asciidoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index e0be70d1e5b7c..832973cbc0580 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -223,8 +223,8 @@ Reduce the number of object allocations made by {security} when resolving the in Respect accept header on requests with no handler ({pull}30383[#30383]) -Stop setting connection request timeout in default low-level REST client configuration because it -causes unexpected timeout exceptions for successful requests ({pull}30384[#30384]) +Stop setting connection request timeout in default low-level REST client configuration because it causes unexpected timeout exceptions for +successful requests ({pull}30384[#30384]) //[float] //=== Regressions From 25b1b1f0b30f60d515c505b0c90217745bc53417 Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 8 May 2018 15:16:26 +0200 Subject: [PATCH 6/7] update changelog --- docs/CHANGELOG.asciidoc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 832973cbc0580..47727a66fd663 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -223,8 +223,9 @@ Reduce the number of object allocations made by {security} when resolving the in Respect accept header on requests with no handler ({pull}30383[#30383]) -Stop setting connection request timeout in default low-level REST client configuration because it causes unexpected timeout exceptions for -successful requests ({pull}30384[#30384]) +Stop setting connection request timeout in default low-level REST client +configuration because it causes unexpected timeout exceptions for successful +requests ({pull}30384[#30384]) //[float] //=== Regressions From 07d24b6280748b145fa45dd35a52405308d2c4dd Mon Sep 17 00:00:00 2001 From: javanna Date: Tue, 8 May 2018 23:40:41 +0200 Subject: [PATCH 7/7] remove changelog entry --- docs/CHANGELOG.asciidoc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 47727a66fd663..621ca5a6414d2 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -223,10 +223,6 @@ Reduce the number of object allocations made by {security} when resolving the in Respect accept header on requests with no handler ({pull}30383[#30383]) -Stop setting connection request timeout in default low-level REST client -configuration because it causes unexpected timeout exceptions for successful -requests ({pull}30384[#30384]) - //[float] //=== Regressions