Update Apache to not reuse connections if they received a 5xx error. (#2960)

This was already the behavior for Netty, and cannot be made the behavior for the URL connection client.

Author: millems

.changes/next-release/bugfix-ApacheHTTPClient-414635a.json

@@ -0,0 +1,6 @@
+{
+    "category": "Apache HTTP Client", 
+    "contributor": "", 
+    "type": "bugfix", 
+    "description": "Do not reuse connections that receive a 5xx service response."
+}

http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/ApacheHttpClient.java

@@ -80,6 +80,7 @@
 import software.amazon.awssdk.http.TlsTrustManagersProvider;
 import software.amazon.awssdk.http.apache.internal.ApacheHttpRequestConfig;
 import software.amazon.awssdk.http.apache.internal.DefaultConfiguration;
+import software.amazon.awssdk.http.apache.internal.SdkConnectionReuseStrategy;
 import software.amazon.awssdk.http.apache.internal.SdkProxyRoutePlanner;
 import software.amazon.awssdk.http.apache.internal.conn.ClientConnectionManagerFactory;
 import software.amazon.awssdk.http.apache.internal.conn.IdleConnectionReaper;
@@ -161,6 +162,7 @@ private ConnectionManagerAwareHttpClient createClient(ApacheHttpClient.DefaultBu
                .disableRedirectHandling()
                .disableAutomaticRetries()
                .setUserAgent("") // SDK will set the user agent header in the pipeline. Don't let Apache waste time
+               .setConnectionReuseStrategy(new SdkConnectionReuseStrategy())
                .setConnectionManager(ClientConnectionManagerFactory.wrap(cm));
 
         addProxyConfig(builder, configuration);

http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/SdkConnectionReuseStrategy.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.http.apache.internal;
+
+import org.apache.http.HttpResponse;
+import org.apache.http.impl.client.DefaultClientConnectionReuseStrategy;
+import org.apache.http.protocol.HttpContext;
+import software.amazon.awssdk.annotations.SdkInternalApi;
+
+/**
+ * Do not reuse connections that returned a 5xx error.
+ *
+ * <p>This is not strictly the behavior we would want in an AWS client, because sometimes we might want to keep a connection open
+ * (e.g. an undocumented service's 503 'SlowDown') and sometimes we might want to close the connection (e.g. S3's 400
+ * RequestTimeout or Glacier's 408 RequestTimeoutException), but this is good enough for the majority of services, and the ones
+ * for which it is not should not be impacted too harshly.
+ */
+@SdkInternalApi
+public class SdkConnectionReuseStrategy extends DefaultClientConnectionReuseStrategy {
+    @Override
+    public boolean keepAlive(HttpResponse response, HttpContext context) {
+        if (!super.keepAlive(response, context)) {
+            return false;
+        }
+
+        if (response == null || response.getStatusLine() == null) {
+            return false;
+        }
+
+        return !is500(response);
+    }
+
+    private boolean is500(HttpResponse httpResponse) {
+        return httpResponse.getStatusLine().getStatusCode() / 100 == 5;
+    }
+}

http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWireMockTest.java

@@ -41,6 +41,11 @@ protected SdkHttpClient createSdkHttpClient(SdkHttpClientOptions options) {
         return builder.buildWithDefaults(attributeMap.build());
     }
 
+    @Override
+    public void connectionsAreNotReusedOn5xxErrors() {
+        // We cannot support this because the URL connection client doesn't allow us to disable connection reuse
+    }
+
     @AfterEach
     public void reset() {
         HttpsURLConnection.setDefaultSSLSocketFactory((SSLSocketFactory) SSLSocketFactory.getDefault());

http-clients/url-connection-client/src/test/java/software/amazon/awssdk/http/urlconnection/UrlConnectionHttpClientWithCustomCreateWireMockTest.java

@@ -55,6 +55,12 @@ public void testTrustAllWorks() {
     public void testCustomTlsTrustManagerAndTrustAllFails() {
     }
 
+    @Disabled
+    @Override
+    public void connectionsAreNotReusedOn5xxErrors() throws Exception {
+        // We cannot support this because the URL connection client doesn't allow us to disable connection reuse
+    }
+
     @Test
     public void testGetResponseCodeNpeIsWrappedAsIo() throws Exception {
         connectionInterceptor = safeFunction(connection -> {

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

@@ -144,6 +144,30 @@ public void connectionPoolingWorks() throws Exception {
         assertThat(CONNECTION_COUNTER.openedConnections()).isEqualTo(initialOpenedConnections + 1);
     }
 
+    @Test
+    public void connectionsAreNotReusedOn5xxErrors() throws Exception {
+        int initialOpenedConnections = CONNECTION_COUNTER.openedConnections();
+
+        SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
+        httpClientOptions.trustAll(true);
+        SdkHttpClient client = createSdkHttpClient(httpClientOptions);
+
+        stubForMockRequest(503);
+
+        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 + 5);
+    }
+
     @Test
     public void testCustomTlsTrustManager() throws Exception {
         WireMockServer selfSignedServer = HttpTestUtils.createSelfSignedServer();