Skip to content

Avoid setting connection request timeout #30384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 9, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nik9000 this test may look familiar to you :)

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}.
Expand Down
2 changes: 2 additions & 0 deletions docs/CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add "because it causes timeout exceptions before any requests are made which isn't how timeouts are supposed to work" or something like that.


//[float]
//=== Regressions

Expand Down