HTTP connection leaked if ExecutionInterceptor throws exception

[jeffrey-easyesi]

Describe the bug

When handling a response with a body, if an ExecutionInterceptor.modifyHttpResponse or ExecutionInterceptor.modifyHttpResponseContent method throws an exception, the HTTP connection is never closed.

Expected Behavior

Any time an AwsClient isn't returning the response stream to the caller, it should close the response itself, releasing the connection back to the pool.

Current Behavior

The connection remains open. This eventually exhausts the connection pool and causes subsequent requests to hang forever.

Reproduction Steps

import com.sun.net.httpserver.HttpServer;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.util.List;
import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration;
import software.amazon.awssdk.core.interceptor.Context;
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
import software.amazon.awssdk.http.SdkHttpRequest;
import software.amazon.awssdk.http.SdkHttpResponse;
import software.amazon.awssdk.services.s3.S3Client;

public class ConnectionLeakDemo {
    public static void main(String[] args) throws IOException {
        HttpServer dummyServer = HttpServer.create(new InetSocketAddress(0), 0);
        ExecutionInterceptor interceptor = new ExecutionInterceptor() {
            @Override
            public SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes) {
                return context.httpRequest().toBuilder()
                        .protocol("http").host("localhost").port(dummyServer.getAddress().getPort())
                        .build();
            }
            @Override
            public SdkHttpResponse modifyHttpResponse(Context.ModifyHttpResponse context, ExecutionAttributes executionAttributes) {
                throw new RuntimeException("Throwing exception from modifyHttpResponse");
            }
        };
        dummyServer.start();
        try (S3Client client = S3Client.builder()
                .overrideConfiguration(ClientOverrideConfiguration.builder().executionInterceptors(List.of(interceptor)).build())
                .build()) {
            for (int i = 1; i <= 100; i++) {
                System.out.print("Request #" + i + ": ");
                try (var in = client.getObject(b -> b.bucket("bucket").key("key"))) {
                    System.out.println(in);
                } catch (Exception e) {
                    System.out.println(e);
                }
            }
        }
        dummyServer.stop(0);
    }
}

After the 50th exception, getObject gets stuck waiting for a connection.

Possible Solution

ExecutionInterceptorChain.modifyHttpResponse could catch all exceptions, and close the current response stream before rethrowing. Something like

+InputStream response = context.responseBody().orElse(null);
+try {
     InterceptorContext result = context;
     for (int i = interceptors.size() - 1; i >= 0; i--) {
         SdkHttpResponse interceptorResult = interceptors.get(i).modifyHttpResponse(result, executionAttributes);
         validateInterceptorResult(result.httpResponse(), interceptorResult, interceptors.get(i), "modifyHttpResponse");
-        InputStream response = interceptors.get(i).modifyHttpResponseContent(result, executionAttributes).orElse(null);
+        response = interceptors.get(i).modifyHttpResponseContent(result, executionAttributes).orElse(null);
         result = result.toBuilder().httpResponse(interceptorResult).responseBody(response).build();
     }
     return result;
+} catch (Throwable t) {
+    IoUtils.closeQuietly(response);
+    throw t;
+}

The problem probably exists in other places, though. I'm not familiar enough with this system to say what a complete solution should be.

Additional Information/Context

It's possible to work around this issue by wrapping all of your own modifyHttpResponse/modifyHttpResponseContent implementations in try/catch/close/rethrow. However, I don't think it's reasonable to expect these methods to close a stream that they didn't create. modifyHttpResponse doesn't even use the stream normally.

AWS Java SDK version used

2.23.4

JDK version used

openjdk version "17.0.10" 2024-01-16

Operating System and version

Ubuntu 20.04