diff --git a/.changes/next-release/bugfix-NettyNIOAsyncHTTPClient-0cfd089.json b/.changes/next-release/bugfix-NettyNIOAsyncHTTPClient-0cfd089.json new file mode 100644 index 000000000000..50cef9dbe904 --- /dev/null +++ b/.changes/next-release/bugfix-NettyNIOAsyncHTTPClient-0cfd089.json @@ -0,0 +1,5 @@ +{ + "category": "Netty NIO Async HTTP Client", + "type": "bugfix", + "description": "Fixes Issue [#340](https://github.com/aws/aws-sdk-java/issues/340) where connection acquisition time was calculated incorrectly in the Netty client." +} diff --git a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfiguration.java b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfiguration.java index 15ee9e586cd1..62470da61309 100644 --- a/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfiguration.java +++ b/http-clients/netty-nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfiguration.java @@ -53,10 +53,8 @@ public int connectionTimeout() { /** * @see NettySdkHttpClientFactory.Builder#maxConnectionsPerEndpoint(Integer) */ - @ReviewBeforeRelease("Does it make sense to use this value? Netty's implementation is max connections" + - " per endpoint so if it's a shared client it doesn't mean quite the same thing.") public int maxConnectionsPerEndpoint() { - return factory.maxConnectionsPerEndpoint().orElseGet(() -> serviceDefaults.get(MAX_CONNECTIONS)); + return serviceDefaults.get(MAX_CONNECTIONS); } @ReviewBeforeRelease("Support disabling strict hostname verification") @@ -85,7 +83,7 @@ public int writeTimeout() { * @see NettySdkHttpClientFactory.Builder#connectionAcquisitionTimeout(Duration) */ public int connectionAcquisitionTimeout() { - return factory.connectionAquisitionTimeout().map(d -> saturatedCast(d.getSeconds())).orElseGet(this::connectionTimeout); + return factory.connectionAquisitionTimeout().map(d -> saturatedCast(d.toMillis())).orElseGet(this::connectionTimeout); } /** diff --git a/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfigurationTest.java b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfigurationTest.java new file mode 100644 index 000000000000..751cc7815da4 --- /dev/null +++ b/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/NettyConfigurationTest.java @@ -0,0 +1,105 @@ +package software.amazon.awssdk.http.nio.netty.internal; + +import static org.assertj.core.api.Assertions.assertThat; +import static software.amazon.awssdk.http.SdkHttpConfigurationOption.CONNECTION_TIMEOUT; +import static software.amazon.awssdk.http.SdkHttpConfigurationOption.MAX_CONNECTIONS; + +import java.time.Duration; +import java.util.function.Consumer; +import org.junit.Test; +import software.amazon.awssdk.http.nio.netty.NettySdkHttpClientFactory; +import software.amazon.awssdk.utils.AttributeMap; + +public class NettyConfigurationTest { + + @Test + public void connectionAcquistion_ReturnsMillis() { + NettyConfiguration configuration = createConfiguration(b -> b.connectionAcquisitionTimeout(Duration.ofSeconds(1))); + assertThat(configuration.connectionAcquisitionTimeout()).isEqualTo(1000); + } + + @Test + public void connectionAcquisition_PerformsSaturatedCast() { + NettyConfiguration configuration = createConfiguration(b -> b.connectionAcquisitionTimeout(Duration.ofMillis(Long.MAX_VALUE))); + assertThat(configuration.connectionAcquisitionTimeout()).isEqualTo(Integer.MAX_VALUE); + } + + @Test + public void connectionTimeout_OnlyPulledFromAttributeMap() { + NettyConfiguration configuration = createConfiguration(a -> a.put(CONNECTION_TIMEOUT, + Duration.ofSeconds(42)), + b -> + b.connectionTimeout(Duration.ofSeconds(9001))); + assertThat(configuration.connectionTimeout()).isEqualTo(42 * 1000); + } + + @Test + public void connectionTimeout_PerformsSaturatedCast() { + NettyConfiguration configuration = createConfiguration(a -> a.put(CONNECTION_TIMEOUT, + Duration.ofMillis(Long.MAX_VALUE)), + b -> { + }); + assertThat(configuration.connectionTimeout()).isEqualTo(Integer.MAX_VALUE); + } + + @Test + public void maxConnectionsPerEndpoint_OnlyPulledFromAttributeMap() { + NettyConfiguration configuration = createConfiguration(a -> a.put(MAX_CONNECTIONS, 10), + b -> b.maxConnectionsPerEndpoint(11)); + assertThat(configuration.maxConnectionsPerEndpoint()).isEqualTo(10); + } + + @Test + public void readTimeout_AppliesDefaultValueIfNotSetOnFactory() { + NettyConfiguration configuration = createEmptyConfiguration(); + assertThat(configuration.readTimeout()).isEqualTo(60); + } + + @Test + public void readTimeout_HonorsFactoryOverDefault() { + NettyConfiguration configuration = createConfiguration(b -> b.readTimeout(Duration.ofSeconds(42))); + assertThat(configuration.readTimeout()).isEqualTo(42); + } + + @Test + public void writeTimeout_AppliesDefaultValueIfNotSetOnFactory() { + NettyConfiguration configuration = createEmptyConfiguration(); + assertThat(configuration.writeTimeout()).isEqualTo(60); + } + + @Test + public void writeTimeout_HonorsFactoryOverDefault() { + NettyConfiguration configuration = createConfiguration(b -> b.writeTimeout(Duration.ofSeconds(42))); + assertThat(configuration.writeTimeout()).isEqualTo(42); + } + + @Test + public void trustAllCertificates_DefaultsToFalse() { + NettyConfiguration configuration = createEmptyConfiguration(); + assertThat(configuration.trustAllCertificates()).isFalse(); + } + + @Test + public void trustAllCertificates_HonorsFactoryOverDefault() { + NettyConfiguration configuration = createConfiguration(b -> b.trustAllCertificates(true)); + assertThat(configuration.trustAllCertificates()).isTrue(); + } + + private NettyConfiguration createEmptyConfiguration() { + return createConfiguration(b -> { + }); + } + + private NettyConfiguration createConfiguration(Consumer builderConsumer) { + return createConfiguration(a -> { + }, builderConsumer); + } + + private NettyConfiguration createConfiguration( + Consumer attributeMapConsumer, + Consumer builderConsumer) { + return new NettyConfiguration(AttributeMap.builder().apply(attributeMapConsumer).build(), + NettySdkHttpClientFactory.builder().apply(builderConsumer).build()); + } + +} \ No newline at end of file