ApacheHttpChannels exposes a Closeable instance (#430)

ApacheHttpChannels exposes a Closeable instance, allowing callers to release resources when they determine the client is no longer needed.

changelog/@unreleased/pr-430.v2.yml

@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: ApacheHttpChannels exposes a Closeable instance, allowing us to release
+    resources when the client is no longer needed.
+  links:
+  - https://github.com/palantir/dialogue/pull/430

dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java

@@ -15,7 +15,6 @@
  */
 package com.palantir.dialogue.hc4;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.Ints;
 import com.palantir.conjure.java.api.config.service.BasicCredentials;
@@ -28,6 +27,8 @@
 import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 import com.palantir.logsafe.exceptions.SafeRuntimeException;
+import java.io.Closeable;
+import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.security.NoSuchAlgorithmException;
@@ -39,6 +40,7 @@
 import java.util.Queue;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLSocketFactory;
@@ -75,14 +77,25 @@ public final class ApacheHttpClientChannels {
     private ApacheHttpClientChannels() {}
 
     public static Channel create(ClientConfiguration conf) {
+        CloseableClient client = createCloseableHttpClient(conf);
+        List<Channel> channels =
+                conf.uris().stream().map(uri -> createSingleUri(uri, client)).collect(Collectors.toList());
+
+        return Channels.create(channels, conf);
+    }
+
+    public static Channel createSingleUri(String uri, CloseableClient client) {
+        return BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client.client, url(uri)));
+    }
+
+    public static CloseableClient createCloseableHttpClient(ClientConfiguration conf) {
         Preconditions.checkArgument(
                 !conf.fallbackToCommonNameVerification(), "fallback-to-common-name-verification is not supported");
         Preconditions.checkArgument(!conf.meshProxy().isPresent(), "Mesh proxy is not supported");
 
         long socketTimeoutMillis =
                 Math.max(conf.readTimeout().toMillis(), conf.writeTimeout().toMillis());
         int connectTimeout = Ints.checkedCast(conf.connectTimeout().toMillis());
-        // TODO(ckozak): close resources?
         HttpClientBuilder builder = HttpClients.custom()
                 .setDefaultRequestConfig(RequestConfig.custom()
                         .setSocketTimeout(Ints.checkedCast(socketTimeoutMillis))
@@ -127,12 +140,27 @@ public static Channel create(ClientConfiguration conf) {
                             .register(AuthSchemes.BASIC, new BasicSchemeFactory())
                             .build());
         });
-        CloseableHttpClient client = builder.build();
-        ImmutableList<Channel> channels = conf.uris().stream()
-                .map(uri -> BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client, url(uri))))
-                .collect(ImmutableList.toImmutableList());
 
-        return Channels.create(channels, conf);
+        return new CloseableClient(builder.build());
+    }
+
+    /** Intentionally opaque wrapper type - we don't want people using the inner Apache client directly. */
+    public static final class CloseableClient implements Closeable {
+        private final CloseableHttpClient client;
+
+        CloseableClient(CloseableHttpClient client) {
+            this.client = client;
+        }
+
+        @Override
+        public void close() throws IOException {
+            client.close();
+        }
+
+        @Override
+        public String toString() {
+            return "SharedResource{client=" + client + '}';
+        }
     }
 
     /**

dialogue-apache-hc4-client/src/test/java/com/palantir/dialogue/hc4/ApacheHttpClientChannelsTest.java

@@ -15,14 +15,71 @@
  */
 package com.palantir.dialogue.hc4;
 
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
 import com.palantir.conjure.java.client.config.ClientConfiguration;
 import com.palantir.dialogue.AbstractChannelTest;
 import com.palantir.dialogue.Channel;
+import com.palantir.dialogue.Endpoint;
+import com.palantir.dialogue.HttpMethod;
+import com.palantir.dialogue.Request;
+import com.palantir.dialogue.Response;
+import com.palantir.dialogue.TestConfigurations;
+import com.palantir.dialogue.UrlBuilder;
+import java.net.UnknownHostException;
+import java.util.Map;
+import org.junit.Test;
 
 public final class ApacheHttpClientChannelsTest extends AbstractChannelTest {
 
     @Override
     protected Channel createChannel(ClientConfiguration config) {
         return ApacheHttpClientChannels.create(config);
     }
+
+    @Test
+    public void close_works() throws Exception {
+        ClientConfiguration conf = TestConfigurations.create("http://foo");
+
+        Channel channel;
+        try (ApacheHttpClientChannels.CloseableClient client =
+                ApacheHttpClientChannels.createCloseableHttpClient(conf)) {
+
+            channel = ApacheHttpClientChannels.createSingleUri("http://foo", client);
+            ListenableFuture<Response> response =
+                    channel.execute(new TestEndpoint(), Request.builder().build());
+            assertThatThrownBy(() -> Futures.getUnchecked(response)).hasCauseInstanceOf(UnknownHostException.class);
+        }
+
+        ListenableFuture<Response> again =
+                channel.execute(new TestEndpoint(), Request.builder().build());
+        assertThatThrownBy(() -> again.get()).hasMessageContaining("Connection pool shut down");
+    }
+
+    private static final class TestEndpoint implements Endpoint {
+        @Override
+        public void renderPath(Map<String, String> _params, UrlBuilder _url) {}
+
+        @Override
+        public HttpMethod httpMethod() {
+            return HttpMethod.GET;
+        }
+
+        @Override
+        public String serviceName() {
+            return "service";
+        }
+
+        @Override
+        public String endpointName() {
+            return "endpoint";
+        }
+
+        @Override
+        public String version() {
+            return "1.0.0";
+        }
+    }
 }