Skip to content

Commit 2590e6e

Browse files
joviegasjoviegas
authored
Move connection timeout configuration from RequestConfig to ConnectionConfig in Apache HttpClient 5 (#6293)
* Replacing deprecated API like connectionTimeout on RequestConfig and passing it via defaultconnectionconfigs * Handle review comments
1 parent 236c102 commit 2590e6e

File tree

7 files changed

+93
-23
lines changed

7 files changed

+93
-23
lines changed

‎http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import org.apache.hc.client5.http.auth.AuthSchemeFactory;
4545
import org.apache.hc.client5.http.auth.CredentialsProvider;
4646
import org.apache.hc.client5.http.classic.methods.HttpUriRequestBase;
47+
import org.apache.hc.client5.http.config.ConnectionConfig;
4748
import org.apache.hc.client5.http.impl.DefaultSchemePortResolver;
4849
import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
4950
import org.apache.hc.client5.http.impl.classic.HttpClients;
@@ -70,6 +71,7 @@
7071
import org.apache.hc.core5.pool.PoolStats;
7172
import org.apache.hc.core5.ssl.SSLInitializationException;
7273
import org.apache.hc.core5.util.TimeValue;
74+
import org.apache.hc.core5.util.Timeout;
7375
import software.amazon.awssdk.annotations.SdkPreviewApi;
7476
import software.amazon.awssdk.annotations.SdkPublicApi;
7577
import software.amazon.awssdk.annotations.SdkTestInternalApi;
@@ -343,7 +345,6 @@ private Apache5HttpRequestConfig createRequestConfig(DefaultBuilder builder,
343345
AttributeMap resolvedOptions) {
344346
return Apache5HttpRequestConfig.builder()
345347
.socketTimeout(resolvedOptions.get(SdkHttpConfigurationOption.READ_TIMEOUT))
346-
.connectionTimeout(resolvedOptions.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT))
347348
.connectionAcquireTimeout(
348349
resolvedOptions.get(SdkHttpConfigurationOption.CONNECTION_ACQUIRE_TIMEOUT))
349350
.proxyConfiguration(builder.proxyConfiguration)
@@ -728,17 +729,28 @@ public PoolingHttpClientConnectionManager create(Apache5HttpClient.DefaultBuilde
728729
.setSSLSocketFactory(sslsf)
729730
.setSchemePortResolver(DefaultSchemePortResolver.INSTANCE)
730731
.setDnsResolver(configuration.dnsResolver);
731-
Duration connectionTtl = standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIME_TO_LIVE);
732-
if (!connectionTtl.isZero()) {
733-
// Skip TTL=0 to maintain backward compatibility (infinite in 4.x vs immediate expiration in 5.x)
734-
builder.setConnectionTimeToLive(TimeValue.of(connectionTtl.toMillis(), TimeUnit.MILLISECONDS));
735-
}
736732
builder.setMaxConnPerRoute(standardOptions.get(SdkHttpConfigurationOption.MAX_CONNECTIONS));
737733
builder.setMaxConnTotal(standardOptions.get(SdkHttpConfigurationOption.MAX_CONNECTIONS));
738734
builder.setDefaultSocketConfig(buildSocketConfig(standardOptions));
735+
builder.setDefaultConnectionConfig(getConnectionConfig(standardOptions));
739736
return builder.build();
740737
}
741738

739+
private static ConnectionConfig getConnectionConfig(AttributeMap standardOptions) {
740+
ConnectionConfig.Builder connectionConfigBuilder =
741+
ConnectionConfig.custom()
742+
.setConnectTimeout(Timeout.ofMilliseconds(
743+
standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT).toMillis()))
744+
.setSocketTimeout(Timeout.ofMilliseconds(
745+
standardOptions.get(SdkHttpConfigurationOption.READ_TIMEOUT).toMillis()));
746+
Duration connectionTtl = standardOptions.get(SdkHttpConfigurationOption.CONNECTION_TIME_TO_LIVE);
747+
if (!connectionTtl.isZero()) {
748+
// Skip TTL=0 to maintain backward compatibility (infinite in 4.x vs immediate expiration in 5.x)
749+
connectionConfigBuilder.setTimeToLive(TimeValue.ofMilliseconds(connectionTtl.toMillis()));
750+
}
751+
return connectionConfigBuilder.build();
752+
}
753+
742754
private SSLConnectionSocketFactory getPreferredSocketFactory(Apache5HttpClient.DefaultBuilder configuration,
743755
AttributeMap standardOptions) {
744756
return Optional.ofNullable(configuration.socketFactory)

‎http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/Apache5HttpRequestConfig.java

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,12 @@
2727
public final class Apache5HttpRequestConfig {
2828

2929
private final Duration socketTimeout;
30-
private final Duration connectionTimeout;
3130
private final Duration connectionAcquireTimeout;
3231
private final boolean expectContinueEnabled;
3332
private final ProxyConfiguration proxyConfiguration;
3433

3534
private Apache5HttpRequestConfig(Builder builder) {
3635
this.socketTimeout = builder.socketTimeout;
37-
this.connectionTimeout = builder.connectionTimeout;
3836
this.connectionAcquireTimeout = builder.connectionAcquireTimeout;
3937
this.expectContinueEnabled = builder.expectContinueEnabled;
4038
this.proxyConfiguration = builder.proxyConfiguration;
@@ -44,10 +42,6 @@ public Duration socketTimeout() {
4442
return socketTimeout;
4543
}
4644

47-
public Duration connectionTimeout() {
48-
return connectionTimeout;
49-
}
50-
5145
public Duration connectionAcquireTimeout() {
5246
return connectionAcquireTimeout;
5347
}
@@ -73,7 +67,6 @@ public static Builder builder() {
7367
public static final class Builder {
7468

7569
private Duration socketTimeout;
76-
private Duration connectionTimeout;
7770
private Duration connectionAcquireTimeout;
7871
private boolean expectContinueEnabled;
7972
private ProxyConfiguration proxyConfiguration;
@@ -86,11 +79,6 @@ public Builder socketTimeout(Duration socketTimeout) {
8679
return this;
8780
}
8881

89-
public Builder connectionTimeout(Duration connectionTimeout) {
90-
this.connectionTimeout = connectionTimeout;
91-
return this;
92-
}
93-
9482
public Builder connectionAcquireTimeout(Duration connectionAcquireTimeout) {
9583
this.connectionAcquireTimeout = connectionAcquireTimeout;
9684
return this;

‎http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/impl/Apache5HttpRequestFactory.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public class Apache5HttpRequestFactory {
5252
private static final List<String> IGNORE_HEADERS = Arrays.asList(HttpHeaders.CONTENT_LENGTH, HttpHeaders.HOST,
5353
HttpHeaders.TRANSFER_ENCODING);
5454

55-
public HttpUriRequestBase create(final HttpExecuteRequest request, final Apache5HttpRequestConfig requestConfig) {
55+
public HttpUriRequestBase create(HttpExecuteRequest request, Apache5HttpRequestConfig requestConfig) {
5656
HttpUriRequestBase base = createApacheRequest(request, sanitizeUri(request.httpRequest()));
5757
addHeadersToRequest(base, request.httpRequest());
5858
addRequestConfig(base, request.httpRequest(), requestConfig);
@@ -90,12 +90,10 @@ private URI sanitizeUri(SdkHttpRequest request) {
9090
private void addRequestConfig(HttpUriRequestBase base,
9191
SdkHttpRequest request,
9292
Apache5HttpRequestConfig requestConfig) {
93-
int connectTimeout = saturatedCast(requestConfig.connectionTimeout().toMillis());
9493
int connectAcquireTimeout = saturatedCast(requestConfig.connectionAcquireTimeout().toMillis());
9594
RequestConfig.Builder requestConfigBuilder = RequestConfig
9695
.custom()
9796
.setConnectionRequestTimeout(connectAcquireTimeout, TimeUnit.MILLISECONDS)
98-
.setConnectTimeout(connectTimeout, TimeUnit.MILLISECONDS)
9997
.setResponseTimeout(saturatedCast(requestConfig.socketTimeout().toMillis()), TimeUnit.MILLISECONDS);
10098

10199
/*

‎http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/internal/utils/Apache5Utils.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ public static CredentialsProvider newProxyCredentialsProvider(ProxyConfiguration
7979
* Returns a new instance of NTCredentials used for proxy authentication.
8080
*/
8181
private static Credentials newNtCredentials(ProxyConfiguration proxyConfiguration) {
82+
// Deprecated NTCredentials is used to maintain backward compatibility with Apache4.
8283
return new NTCredentials(
8384
proxyConfiguration.username(),
8485
proxyConfiguration.password().toCharArray(),

‎http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
1919
import static com.github.tomakehurst.wiremock.client.WireMock.any;
20+
import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
2021
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo;
2122
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
2223
import static org.assertj.core.api.Assertions.assertThat;
@@ -28,10 +29,15 @@
2829
import com.github.tomakehurst.wiremock.junit.WireMockRule;
2930
import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
3031
import java.io.IOException;
32+
import java.net.ConnectException;
3133
import java.net.HttpURLConnection;
3234
import java.net.InetAddress;
35+
import java.net.SocketTimeoutException;
3336
import java.net.URI;
3437
import java.net.UnknownHostException;
38+
import java.time.Duration;
39+
import java.util.stream.Stream;
40+
import org.apache.hc.client5.http.ConnectTimeoutException;
3541
import org.apache.hc.client5.http.DnsResolver;
3642
import org.apache.hc.client5.http.HttpRoute;
3743
import org.apache.hc.client5.http.SystemDefaultDnsResolver;
@@ -279,4 +285,71 @@ public void closeReleasesResources() throws Exception {
279285
IllegalStateException.class
280286
).hasMessageContaining("Connection pool shut down");
281287
}
288+
289+
@Test
290+
public void connectionTimeout_exceedsLimit_throwsException() throws Exception {
291+
// Test connection timeout with a very short timeout and non-responsive address
292+
try (SdkHttpClient client = Apache5HttpClient.builder()
293+
.connectionTimeout(Duration.ofMillis(100))
294+
.build()) {
295+
296+
// Use a non-routable address to simulate connection timeout
297+
// 192.0.2.1 is a reserved test address
298+
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
299+
.uri(URI.create("http://192.0.2.1:8080/test"))
300+
.method(SdkHttpMethod.GET)
301+
.putHeader("Host", "192.0.2.1:8080")
302+
.build();
303+
304+
assertThatThrownBy(() ->
305+
client.prepareRequest(HttpExecuteRequest.builder().request(request).build()).call())
306+
.isInstanceOfAny(
307+
ConnectTimeoutException.class,
308+
ConnectException.class,
309+
IOException.class)
310+
.satisfies(exception -> {
311+
// message vary based on JVM
312+
String message = exception.getMessage().toLowerCase();
313+
boolean hasTimeoutMessage = Stream.of("timeout", "timed out", "read timeout")
314+
.anyMatch(message::contains);
315+
assertThat(hasTimeoutMessage).isTrue();
316+
});
317+
}
318+
}
319+
320+
@Test
321+
public void socketTimeout_exceedsLimit_throwsException() throws Exception {
322+
// Configure WireMock to delay response longer than socket timeout
323+
mockServer.stubFor(any(urlPathEqualTo("/delayed"))
324+
.willReturn(aResponse()
325+
.withStatus(200)
326+
.withBody("delayed response")
327+
.withFixedDelay(2000)));
328+
329+
try (SdkHttpClient client = Apache5HttpClient.builder()
330+
.socketTimeout(Duration.ofMillis(500))
331+
.build()) {
332+
333+
SdkHttpFullRequest request = SdkHttpFullRequest.builder()
334+
.uri(URI.create("http://localhost:" + mockServer.port() + "/delayed"))
335+
.method(SdkHttpMethod.GET)
336+
.putHeader("Host", "localhost:" + mockServer.port())
337+
.putHeader("User-Agent", "test-client")
338+
.build();
339+
340+
assertThatThrownBy(() ->
341+
client.prepareRequest(HttpExecuteRequest.builder().request(request).build()).call())
342+
.isInstanceOfAny(
343+
SocketTimeoutException.class,
344+
IOException.class)
345+
.satisfies(exception -> {
346+
String message = exception.getMessage().toLowerCase();
347+
boolean hasTimeoutMessage = Stream.of("timeout", "timed out", "read timeout")
348+
.anyMatch(message::contains);
349+
assertThat(hasTimeoutMessage).isTrue();
350+
351+
});
352+
mockServer.verify(1, getRequestedFor(urlPathEqualTo("/delayed")));
353+
}
354+
}
282355
}

‎http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,6 @@ public void prepareRequest_connectionManagerNotPooling_callableCalled_metricsRep
109109
private Apache5HttpClient newClient() {
110110
Apache5HttpRequestConfig config = Apache5HttpRequestConfig.builder()
111111
.connectionAcquireTimeout(Duration.ofDays(1))
112-
.connectionTimeout(Duration.ofDays(1))
113112
.socketTimeout(Duration.ofDays(1))
114113
.proxyConfiguration(ProxyConfiguration.builder().build())
115114
.build();

‎http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/internal/impl/ApacheHttpRequestFactoryTest.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ public void setup() {
5050
instance = new Apache5HttpRequestFactory();
5151
requestConfig = Apache5HttpRequestConfig.builder()
5252
.connectionAcquireTimeout(Duration.ZERO)
53-
.connectionTimeout(Duration.ZERO)
5453
.socketTimeout(Duration.ZERO)
5554
.build();
5655
}

0 commit comments

Comments
 (0)