Fixing Issue #340

.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."
+}

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);
     }
 
     /**

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<NettySdkHttpClientFactory.Builder> builderConsumer) {
+        return createConfiguration(a -> {
+        }, builderConsumer);
+    }
+
+    private NettyConfiguration createConfiguration(
+        Consumer<AttributeMap.Builder> attributeMapConsumer,
+        Consumer<NettySdkHttpClientFactory.Builder> builderConsumer) {
+        return new NettyConfiguration(AttributeMap.builder().apply(attributeMapConsumer).build(),
+                                      NettySdkHttpClientFactory.builder().apply(builderConsumer).build());
+    }
+
+}