Fixed connection pooling in the UrlConnectionHttpClient. (#2660)

Author: millems

.changes/next-release/bugfix-URLConnectionHttpClient-3542fcd.json

@@ -0,0 +1,6 @@
+{
+    "category": "URL Connection Http Client", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Fixed connection pooling for HTTPS endpoints. Previously, each request would create a new connection."
+}

http-clients/url-connection-client/src/main/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClient.java

@@ -36,6 +36,7 @@
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLSession;
+import javax.net.ssl.SSLSocketFactory;
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.X509TrustManager;
 import software.amazon.awssdk.annotations.SdkPublicApi;
@@ -71,18 +72,18 @@ public final class UrlConnectionHttpClient implements SdkHttpClient {
 
     private final AttributeMap options;
     private final UrlConnectionFactory connectionFactory;
-    private final SSLContext sslContext;
 
     private UrlConnectionHttpClient(AttributeMap options, UrlConnectionFactory connectionFactory) {
         this.options = options;
         if (connectionFactory != null) {
-            this.sslContext = null;
             this.connectionFactory = connectionFactory;
         } else {
-            this.sslContext = getSslContext(options);
-            this.connectionFactory = this::createDefaultConnection;
-        }
+            // Note: This socket factory MUST be reused between requests because the connection pool in the JVM is keyed by both
+            // URL and SSLSocketFactory. If the socket factory is not reused, connections will not be reused between requests.
+            SSLSocketFactory socketFactory = getSslContext(options).getSocketFactory();
 
+            this.connectionFactory = url -> createDefaultConnection(url, socketFactory);
+        }
     }
 
     public static Builder builder() {
@@ -142,7 +143,7 @@ private HttpURLConnection createAndConfigureConnection(HttpExecuteRequest reques
         return connection;
     }
 
-    private HttpURLConnection createDefaultConnection(URI uri) {
+    private HttpURLConnection createDefaultConnection(URI uri, SSLSocketFactory socketFactory) {
         HttpURLConnection connection = invokeSafely(() -> (HttpURLConnection) uri.toURL().openConnection());
 
         if (connection instanceof HttpsURLConnection) {
@@ -152,7 +153,7 @@ private HttpURLConnection createDefaultConnection(URI uri) {
                 httpsConnection.setHostnameVerifier(NoOpHostNameVerifier.INSTANCE);
             }
 
-            httpsConnection.setSSLSocketFactory(sslContext.getSocketFactory());
+            httpsConnection.setSSLSocketFactory(socketFactory);
         }
 
         connection.setConnectTimeout(saturatedCast(options.get(SdkHttpConfigurationOption.CONNECTION_TIMEOUT).toMillis()));

test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java

@@ -29,14 +29,18 @@
 import com.github.tomakehurst.wiremock.client.ResponseDefinitionBuilder;
 import com.github.tomakehurst.wiremock.common.FatalStartupException;
 import com.github.tomakehurst.wiremock.http.RequestMethod;
+import com.github.tomakehurst.wiremock.http.trafficlistener.WiremockNetworkTrafficListener;
 import com.github.tomakehurst.wiremock.junit.WireMockRule;
 import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.net.HttpURLConnection;
+import java.net.Socket;
 import java.net.URI;
+import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.Random;
+import java.util.concurrent.atomic.AtomicInteger;
 import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
@@ -57,11 +61,14 @@
 public abstract class SdkHttpClientTestSuite {
     private static final Logger LOG = Logger.loggerFor(SdkHttpClientTestSuite.class);
 
+    private static final ConnectionCountingTrafficListener CONNECTION_COUNTER = new ConnectionCountingTrafficListener();
+
     @Rule
     public WireMockRule mockServer = createWireMockRule();
 
     private final Random rng = new Random();
 
+
     @Test
     public void supportsResponseCode200() throws Exception {
         testForResponseCode(HttpURLConnection.HTTP_OK);
@@ -113,6 +120,30 @@ public void validatesHttpsCertificateIssuer() throws Exception {
                 .isInstanceOf(SSLHandshakeException.class);
     }
 
+    @Test
+    public void connectionPoolingWorks() throws Exception {
+        int initialOpenedConnections = CONNECTION_COUNTER.openedConnections();
+
+        SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
+        httpClientOptions.trustAll(true);
+        SdkHttpClient client = createSdkHttpClient(httpClientOptions);
+
+        stubForMockRequest(200);
+
+        for (int i = 0; i < 5; i++) {
+            SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
+            HttpExecuteResponse response =
+                client.prepareRequest(HttpExecuteRequest.builder()
+                                                        .request(req)
+                                                        .contentStreamProvider(req.contentStreamProvider().orElse(null))
+                                                        .build())
+                      .call();
+            response.responseBody().ifPresent(IoUtils::drainInputStream);
+        }
+
+        assertThat(CONNECTION_COUNTER.openedConnections()).isEqualTo(initialOpenedConnections + 1);
+    }
+
     @Test
     public void testCustomTlsTrustManager() throws Exception {
         WireMockServer selfSignedServer = HttpTestUtils.createSelfSignedServer();
@@ -279,7 +310,9 @@ private WireMockRule createWireMockRule() {
         int maxAttempts = 5;
         for (int i = 0; i < maxAttempts; ++i) {
             try {
-                return new WireMockRule(wireMockConfig().dynamicPort().dynamicHttpsPort());
+                return new WireMockRule(wireMockConfig().dynamicPort()
+                                                        .dynamicHttpsPort()
+                                                        .networkTrafficListener(CONNECTION_COUNTER));
             } catch (FatalStartupException e) {
                 int attemptNum = i + 1;
                 LOG.debug(() -> "Was not able to start WireMock server. Attempt " + attemptNum, e);
@@ -298,4 +331,29 @@ private WireMockRule createWireMockRule() {
 
         throw new RuntimeException("Unable to setup WireMock rule");
     }
+
+    private static class ConnectionCountingTrafficListener implements WiremockNetworkTrafficListener {
+        private final AtomicInteger openedConnections = new AtomicInteger(0);
+
+        @Override
+        public void opened(Socket socket) {
+            openedConnections.incrementAndGet();
+        }
+
+        @Override
+        public void incoming(Socket socket, ByteBuffer bytes) {
+        }
+
+        @Override
+        public void outgoing(Socket socket, ByteBuffer bytes) {
+        }
+
+        @Override
+        public void closed(Socket socket) {
+        }
+
+        public int openedConnections() {
+            return openedConnections.get();
+        }
+    }
 }