Skip to content

Move connection timeout configuration from RequestConfig to ConnectionConfig in Apache HttpClient 5 #6293

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
Show file tree
Hide file tree
Changes from all 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 @@ -44,6 +44,7 @@
import org.apache.hc.client5.http.auth.AuthSchemeFactory;
import org.apache.hc.client5.http.auth.CredentialsProvider;
import org.apache.hc.client5.http.classic.methods.HttpUriRequestBase;
import org.apache.hc.client5.http.config.ConnectionConfig;
import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
import org.apache.hc.client5.http.impl.classic.HttpClients;
Expand All @@ -70,6 +71,7 @@
import org.apache.hc.core5.pool.PoolStats;
import org.apache.hc.core5.ssl.SSLInitializationException;
import org.apache.hc.core5.util.TimeValue;
import org.apache.hc.core5.util.Timeout;
import software.amazon.awssdk.annotations.SdkPreviewApi;
import software.amazon.awssdk.annotations.SdkPublicApi;
import software.amazon.awssdk.annotations.SdkTestInternalApi;
Expand Down Expand Up @@ -343,7 +345,6 @@ private Apache5HttpRequestConfig createRequestConfig(DefaultBuilder builder,
AttributeMap resolvedOptions) {
return Apache5HttpRequestConfig.builder()
.socketTimeout(resolvedOptions.get(SdkHttpConfigurationOption.READ_TIMEOUT))
.connectionTimeout(resolvedOptions.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT))
.connectionAcquireTimeout(
resolvedOptions.get(SdkHttpConfigurationOption.CONNECTION_ACQUIRE_TIMEOUT))
.proxyConfiguration(builder.proxyConfiguration)
Expand Down Expand Up @@ -728,17 +729,28 @@ public PoolingHttpClientConnectionManager create(Apache5HttpClient.DefaultBuilde
.setSSLSocketFactory(sslsf)
.setSchemePortResolver(DefaultSchemePortResolver.INSTANCE)
.setDnsResolver(configuration.dnsResolver);
Duration connectionTtl = standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIME_TO_LIVE);
if (!connectionTtl.isZero()) {
// Skip TTL=0 to maintain backward compatibility (infinite in 4.x vs immediate expiration in 5.x)
builder.setConnectionTimeToLive(TimeValue.of(connectionTtl.toMillis(), TimeUnit.MILLISECONDS));
}
builder.setMaxConnPerRoute(standardOptions.get(SdkHttpConfigurationOption.MAX_CONNECTIONS));
builder.setMaxConnTotal(standardOptions.get(SdkHttpConfigurationOption.MAX_CONNECTIONS));
builder.setDefaultSocketConfig(buildSocketConfig(standardOptions));
builder.setDefaultConnectionConfig(getConnectionConfig(standardOptions));
return builder.build();
}

private static ConnectionConfig getConnectionConfig(AttributeMap standardOptions) {
ConnectionConfig.Builder connectionConfigBuilder =
ConnectionConfig.custom()
.setConnectTimeout(Timeout.ofMilliseconds(
standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT).toMillis()))
.setSocketTimeout(Timeout.ofMilliseconds(
standardOptions.get(SdkHttpConfigurationOption.READ_TIMEOUT).toMillis()));
Duration connectionTtl = standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIME_TO_LIVE);
if (!connectionTtl.isZero()) {
// Skip TTL=0 to maintain backward compatibility (infinite in 4.x vs immediate expiration in 5.x)
connectionConfigBuilder.setTimeToLive(TimeValue.ofMilliseconds(connectionTtl.toMillis()));
}
return connectionConfigBuilder.build();
}

private SSLConnectionSocketFactory getPreferredSocketFactory(Apache5HttpClient.DefaultBuilder configuration,
AttributeMap standardOptions) {
return Optional.ofNullable(configuration.socketFactory)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,12 @@
public final class Apache5HttpRequestConfig {

private final Duration socketTimeout;
private final Duration connectionTimeout;
private final Duration connectionAcquireTimeout;
private final boolean expectContinueEnabled;
private final ProxyConfiguration proxyConfiguration;

private Apache5HttpRequestConfig(Builder builder) {
this.socketTimeout = builder.socketTimeout;
this.connectionTimeout = builder.connectionTimeout;
this.connectionAcquireTimeout = builder.connectionAcquireTimeout;
this.expectContinueEnabled = builder.expectContinueEnabled;
this.proxyConfiguration = builder.proxyConfiguration;
Expand All @@ -44,10 +42,6 @@ public Duration socketTimeout() {
return socketTimeout;
}

public Duration connectionTimeout() {
return connectionTimeout;
}

public Duration connectionAcquireTimeout() {
return connectionAcquireTimeout;
}
Expand All @@ -73,7 +67,6 @@ public static Builder builder() {
public static final class Builder {

private Duration socketTimeout;
private Duration connectionTimeout;
private Duration connectionAcquireTimeout;
private boolean expectContinueEnabled;
private ProxyConfiguration proxyConfiguration;
Expand All @@ -86,11 +79,6 @@ public Builder socketTimeout(Duration socketTimeout) {
return this;
}

public Builder connectionTimeout(Duration connectionTimeout) {
this.connectionTimeout = connectionTimeout;
return this;
}

public Builder connectionAcquireTimeout(Duration connectionAcquireTimeout) {
this.connectionAcquireTimeout = connectionAcquireTimeout;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public class Apache5HttpRequestFactory {
private static final List<String> IGNORE_HEADERS = Arrays.asList(HttpHeaders.CONTENT_LENGTH, HttpHeaders.HOST,
HttpHeaders.TRANSFER_ENCODING);

public HttpUriRequestBase create(final HttpExecuteRequest request, final Apache5HttpRequestConfig requestConfig) {
public HttpUriRequestBase create(HttpExecuteRequest request, Apache5HttpRequestConfig requestConfig) {
HttpUriRequestBase base = createApacheRequest(request, sanitizeUri(request.httpRequest()));
addHeadersToRequest(base, request.httpRequest());
addRequestConfig(base, request.httpRequest(), requestConfig);
Expand Down Expand Up @@ -90,12 +90,10 @@ private URI sanitizeUri(SdkHttpRequest request) {
private void addRequestConfig(HttpUriRequestBase base,
SdkHttpRequest request,
Apache5HttpRequestConfig requestConfig) {
int connectTimeout = saturatedCast(requestConfig.connectionTimeout().toMillis());
int connectAcquireTimeout = saturatedCast(requestConfig.connectionAcquireTimeout().toMillis());
RequestConfig.Builder requestConfigBuilder = RequestConfig
.custom()
.setConnectionRequestTimeout(connectAcquireTimeout, TimeUnit.MILLISECONDS)
.setConnectTimeout(connectTimeout, TimeUnit.MILLISECONDS)
.setResponseTimeout(saturatedCast(requestConfig.socketTimeout().toMillis()), TimeUnit.MILLISECONDS);

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public static CredentialsProvider newProxyCredentialsProvider(ProxyConfiguration
* Returns a new instance of NTCredentials used for proxy authentication.
*/
private static Credentials newNtCredentials(ProxyConfiguration proxyConfiguration) {
// Deprecated NTCredentials is used to maintain backward compatibility with Apache4.
return new NTCredentials(
proxyConfiguration.username(),
proxyConfiguration.password().toCharArray(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
import static com.github.tomakehurst.wiremock.client.WireMock.any;
import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo;
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -28,10 +29,15 @@
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
import java.io.IOException;
import java.net.ConnectException;
import java.net.HttpURLConnection;
import java.net.InetAddress;
import java.net.SocketTimeoutException;
import java.net.URI;
import java.net.UnknownHostException;
import java.time.Duration;
import java.util.stream.Stream;
import org.apache.hc.client5.http.ConnectTimeoutException;
import org.apache.hc.client5.http.DnsResolver;
import org.apache.hc.client5.http.HttpRoute;
import org.apache.hc.client5.http.SystemDefaultDnsResolver;
Expand Down Expand Up @@ -279,4 +285,71 @@ public void closeReleasesResources() throws Exception {
IllegalStateException.class
).hasMessageContaining("Connection pool shut down");
}

@Test
public void connectionTimeout_exceedsLimit_throwsException() throws Exception {
// Test connection timeout with a very short timeout and non-responsive address
try (SdkHttpClient client = Apache5HttpClient.builder()
.connectionTimeout(Duration.ofMillis(100))
.build()) {

// Use a non-routable address to simulate connection timeout
// 192.0.2.1 is a reserved test address
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
.uri(URI.create("http://192.0.2.1:8080/test"))
.method(SdkHttpMethod.GET)
.putHeader("Host", "192.0.2.1:8080")
.build();

assertThatThrownBy(() ->
client.prepareRequest(HttpExecuteRequest.builder().request(request).build()).call())
.isInstanceOfAny(
ConnectTimeoutException.class,
ConnectException.class,
IOException.class)
.satisfies(exception -> {
// message vary based on JVM
String message = exception.getMessage().toLowerCase();
boolean hasTimeoutMessage = Stream.of("timeout", "timed out", "read timeout")
.anyMatch(message::contains);
assertThat(hasTimeoutMessage).isTrue();
});
}
}

@Test
public void socketTimeout_exceedsLimit_throwsException() throws Exception {
// Configure WireMock to delay response longer than socket timeout
mockServer.stubFor(any(urlPathEqualTo("/delayed"))
.willReturn(aResponse()
.withStatus(200)
.withBody("delayed response")
.withFixedDelay(2000)));

try (SdkHttpClient client = Apache5HttpClient.builder()
.socketTimeout(Duration.ofMillis(500))
.build()) {

SdkHttpFullRequest request = SdkHttpFullRequest.builder()
.uri(URI.create("http://localhost:" + mockServer.port() + "/delayed"))
.method(SdkHttpMethod.GET)
.putHeader("Host", "localhost:" + mockServer.port())
.putHeader("User-Agent", "test-client")
.build();

assertThatThrownBy(() ->
client.prepareRequest(HttpExecuteRequest.builder().request(request).build()).call())
.isInstanceOfAny(
SocketTimeoutException.class,
IOException.class)
.satisfies(exception -> {
String message = exception.getMessage().toLowerCase();
boolean hasTimeoutMessage = Stream.of("timeout", "timed out", "read timeout")
.anyMatch(message::contains);
assertThat(hasTimeoutMessage).isTrue();

});
mockServer.verify(1, getRequestedFor(urlPathEqualTo("/delayed")));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ public void prepareRequest_connectionManagerNotPooling_callableCalled_metricsRep
private Apache5HttpClient newClient() {
Apache5HttpRequestConfig config = Apache5HttpRequestConfig.builder()
.connectionAcquireTimeout(Duration.ofDays(1))
.connectionTimeout(Duration.ofDays(1))
.socketTimeout(Duration.ofDays(1))
.proxyConfiguration(ProxyConfiguration.builder().build())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ public void setup() {
instance = new Apache5HttpRequestFactory();
requestConfig = Apache5HttpRequestConfig.builder()
.connectionAcquireTimeout(Duration.ZERO)
.connectionTimeout(Duration.ZERO)
.socketTimeout(Duration.ZERO)
.build();
}
Expand Down
Loading