ApacheHttpChannels exposes a Closeable instance

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;
@@ -40,6 +39,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,15 +75,26 @@ public final class ApacheHttpClientChannels {
 
     private ApacheHttpClientChannels() {}
 
-    public static Channel create(ClientConfiguration conf, UserAgent baseAgent) {
+    public static Channel create(ClientConfiguration conf, UserAgent agent) {
+        CloseableHttpClient client = createCloseableHttpClient(conf);
+        List<Channel> channels =
+                conf.uris().stream().map(uri -> createSingleUri(uri, client)).collect(Collectors.toList());
+
+        return Channels.create(channels, agent, conf);
+    }
+
+    public static Channel createSingleUri(String uri, CloseableHttpClient client) {
+        return BlockingChannelAdapter.of(new ApacheHttpClientBlockingChannel(client, url(uri)));
+    }
+
+    public static CloseableHttpClient 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))
@@ -128,12 +139,8 @@ public static Channel create(ClientConfiguration conf, UserAgent baseAgent) {
                             .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, baseAgent, conf);
+        return builder.build();
     }
 
     /**

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

@@ -15,15 +15,72 @@
  */
 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.api.config.service.UserAgent;
 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.apache.http.impl.client.CloseableHttpClient;
+import org.junit.Test;
 
 public final class ApacheHttpClientChannelsTest extends AbstractChannelTest {
 
     @Override
     protected Channel createChannel(ClientConfiguration config, UserAgent agent) {
         return ApacheHttpClientChannels.create(config, agent);
     }
+
+    @Test
+    public void close_works() throws Exception {
+        ClientConfiguration conf = TestConfigurations.create("http://foo");
+
+        Channel channel;
+        try (CloseableHttpClient 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";
+        }
+    }
 }