diff --git a/changelog/@unreleased/pr-430.v2.yml b/changelog/@unreleased/pr-430.v2.yml new file mode 100644 index 000000000..5db9088a6 --- /dev/null +++ b/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 diff --git a/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java b/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java index f3856ff37..8b1d78723 100644 --- a/dialogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java +++ b/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,6 +77,18 @@ public final class ApacheHttpClientChannels { private ApacheHttpClientChannels() {} public static Channel create(ClientConfiguration conf) { + CloseableClient client = createCloseableHttpClient(conf); + List 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"); @@ -82,7 +96,6 @@ public static Channel create(ClientConfiguration conf) { 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 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 + '}'; + } } /** diff --git a/dialogue-apache-hc4-client/src/test/java/com/palantir/dialogue/hc4/ApacheHttpClientChannelsTest.java b/dialogue-apache-hc4-client/src/test/java/com/palantir/dialogue/hc4/ApacheHttpClientChannelsTest.java index 597c9623c..f9a1e38ba 100644 --- a/dialogue-apache-hc4-client/src/test/java/com/palantir/dialogue/hc4/ApacheHttpClientChannelsTest.java +++ b/dialogue-apache-hc4-client/src/test/java/com/palantir/dialogue/hc4/ApacheHttpClientChannelsTest.java @@ -15,9 +15,22 @@ */ 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 { @@ -25,4 +38,48 @@ public final class ApacheHttpClientChannelsTest extends AbstractChannelTest { 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 = + channel.execute(new TestEndpoint(), Request.builder().build()); + assertThatThrownBy(() -> Futures.getUnchecked(response)).hasCauseInstanceOf(UnknownHostException.class); + } + + ListenableFuture 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 _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"; + } + } }