From 8eae14dfcc369b98a147254b21c453ff2427248e Mon Sep 17 00:00:00 2001 From: samvaity Date: Mon, 25 Oct 2021 10:21:38 -0700 Subject: [PATCH 1/3] use a configuration to enable sharing of http cleints --- .../http/jdk/httpclient/JdkHttpClientProvider.java | 8 +------- .../http/netty/NettyAsyncHttpClientProvider.java | 12 +++++++++--- .../core/http/okhttp/OkHttpAsyncClientProvider.java | 10 ++++++++-- .../main/java/com/azure/core/util/Configuration.java | 8 +++++++- 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java index 035a5500e17d..9b86327c7df0 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java +++ b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java @@ -6,8 +6,6 @@ import com.azure.core.http.HttpClient; import com.azure.core.http.HttpClientProvider; -import java.util.concurrent.atomic.AtomicReference; - /** * An {@link HttpClientProvider} that provides an implementation of HttpClient based on native JDK HttpClient. *

@@ -15,12 +13,8 @@ * introduced. */ public final class JdkHttpClientProvider implements HttpClientProvider { - private static final AtomicReference DEFAULT_HTTP_CLIENT = new AtomicReference<>(); - @Override public HttpClient createInstance() { - DEFAULT_HTTP_CLIENT.compareAndSet(null, new JdkAsyncHttpClientBuilder().build()); - - return DEFAULT_HTTP_CLIENT.get(); + return new JdkAsyncHttpClientBuilder().build(); } } diff --git a/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java b/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java index f503e9cef550..adbd768ba2b0 100644 --- a/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java +++ b/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java @@ -18,9 +18,10 @@ public final class NettyAsyncHttpClientProvider implements HttpClientProvider { @Override public HttpClient createInstance() { - DEFAULT_HTTP_CLIENT.compareAndSet(null, new NettyAsyncHttpClientBuilder().build()); - - return DEFAULT_HTTP_CLIENT.get(); + HttpClient httpClient = new NettyAsyncHttpClientBuilder().build(); + DEFAULT_HTTP_CLIENT.compareAndSet(null, httpClient); + // by default use a singleton http client + return httpClient; } @Override @@ -28,6 +29,11 @@ public HttpClient createInstance(HttpClientOptions clientOptions) { NettyAsyncHttpClientBuilder builder = new NettyAsyncHttpClientBuilder(); if (clientOptions != null) { + if (clientOptions.getConfiguration() != null) { + DEFAULT_HTTP_CLIENT.compareAndSet(null, new NettyAsyncHttpClientBuilder().build()); + return DEFAULT_HTTP_CLIENT.get(); + } + builder = builder.proxy(clientOptions.getProxyOptions()) .configuration(clientOptions.getConfiguration()) .writeTimeout(clientOptions.getWriteTimeout()) diff --git a/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java b/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java index a9333c5d2892..248050f101f0 100644 --- a/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java +++ b/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java @@ -19,9 +19,10 @@ public final class OkHttpAsyncClientProvider implements HttpClientProvider { @Override public HttpClient createInstance() { + HttpClient httpClient = new OkHttpAsyncHttpClientBuilder().build(); DEFAULT_HTTP_CLIENT.compareAndSet(null, new OkHttpAsyncHttpClientBuilder().build()); - - return DEFAULT_HTTP_CLIENT.get(); + // by default use a singleton http client + return httpClient; } @Override @@ -29,6 +30,11 @@ public HttpClient createInstance(HttpClientOptions clientOptions) { OkHttpAsyncHttpClientBuilder builder = new OkHttpAsyncHttpClientBuilder(); if (clientOptions != null) { + if (clientOptions.getConfiguration() != null) { + DEFAULT_HTTP_CLIENT.compareAndSet(null, new OkHttpAsyncHttpClientBuilder().build()); + return DEFAULT_HTTP_CLIENT.get(); + } + builder = builder.proxy(clientOptions.getProxyOptions()) .configuration(clientOptions.getConfiguration()) .writeTimeout(clientOptions.getWriteTimeout()) diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java b/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java index 29de34d5a17c..668f8ad26e19 100644 --- a/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java +++ b/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java @@ -133,6 +133,11 @@ public class Configuration implements Cloneable { */ public static final String PROPERTY_AZURE_TRACING_DISABLED = "AZURE_TRACING_DISABLED"; + /** + * Flag to disable sharing of http clients if not configured by the user. + */ + public static final String PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED = "PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED"; + /* * Configurations that are loaded into the global configuration store when the application starts. */ @@ -159,7 +164,8 @@ public class Configuration implements Cloneable { PROPERTY_AZURE_HTTP_LOG_DETAIL_LEVEL, PROPERTY_AZURE_TRACING_DISABLED, PROPERTY_AZURE_POD_IDENTITY_TOKEN_URL, - PROPERTY_AZURE_REGIONAL_AUTHORITY_NAME + PROPERTY_AZURE_REGIONAL_AUTHORITY_NAME, + PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED }; /* From 8e9078b05a1542943a35658d26e7b6824f7b742e Mon Sep 17 00:00:00 2001 From: samvaity Date: Mon, 25 Oct 2021 16:33:25 -0700 Subject: [PATCH 2/3] update to use default singleton --- .../jdk/httpclient/JdkHttpClientProvider.java | 9 ++++++++- .../jdk/httpclient/JdkAsyncHttpClientTests.java | 7 +++++++ .../netty/NettyAsyncHttpClientProvider.java | 11 +++-------- .../http/netty/NettyAsyncHttpClientTests.java | 17 +++++++++++++++++ .../http/okhttp/OkHttpAsyncClientProvider.java | 17 ++++++----------- .../http/okhttp/OkHttpAsyncHttpClientTests.java | 17 +++++++++++++++++ .../java/com/azure/core/util/Configuration.java | 8 +------- 7 files changed, 59 insertions(+), 27 deletions(-) diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java index 9b86327c7df0..e65e7c1fdb06 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java +++ b/sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java @@ -6,6 +6,8 @@ import com.azure.core.http.HttpClient; import com.azure.core.http.HttpClientProvider; +import java.util.concurrent.atomic.AtomicReference; + /** * An {@link HttpClientProvider} that provides an implementation of HttpClient based on native JDK HttpClient. *

@@ -13,8 +15,13 @@ * introduced. */ public final class JdkHttpClientProvider implements HttpClientProvider { + private static final AtomicReference DEFAULT_HTTP_CLIENT = new AtomicReference<>(); + @Override public HttpClient createInstance() { - return new JdkAsyncHttpClientBuilder().build(); + // by default use a singleton instance of http client + DEFAULT_HTTP_CLIENT.compareAndSet(null, new JdkAsyncHttpClientBuilder().build()); + + return DEFAULT_HTTP_CLIENT.get(); } } diff --git a/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkAsyncHttpClientTests.java b/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkAsyncHttpClientTests.java index eebaadd5fe57..7488b5182ae3 100644 --- a/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkAsyncHttpClientTests.java +++ b/sdk/core/azure-core-http-jdk-httpclient/src/test/java/com/azure/core/http/jdk/httpclient/JdkAsyncHttpClientTests.java @@ -234,6 +234,13 @@ public void testConcurrentRequests() throws NoSuchAlgorithmException { // assertEquals(numRequests * LONG_BODY.getBytes(StandardCharsets.UTF_8).length, numBytes); } + @Test + public void testSingletonClientInstanceCreation() { + HttpClient client1 = new JdkHttpClientProvider().createInstance(); + HttpClient client2 = new JdkHttpClientProvider().createInstance(); + Assertions.assertEquals(client1, client2); + } + private static MessageDigest md5Digest() { try { return MessageDigest.getInstance("MD5"); diff --git a/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java b/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java index adbd768ba2b0..73b959a888d8 100644 --- a/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java +++ b/sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java @@ -18,10 +18,9 @@ public final class NettyAsyncHttpClientProvider implements HttpClientProvider { @Override public HttpClient createInstance() { - HttpClient httpClient = new NettyAsyncHttpClientBuilder().build(); - DEFAULT_HTTP_CLIENT.compareAndSet(null, httpClient); - // by default use a singleton http client - return httpClient; + // by default use a singleton instance of http client + DEFAULT_HTTP_CLIENT.compareAndSet(null, new NettyAsyncHttpClientBuilder().build()); + return DEFAULT_HTTP_CLIENT.get(); } @Override @@ -29,10 +28,6 @@ public HttpClient createInstance(HttpClientOptions clientOptions) { NettyAsyncHttpClientBuilder builder = new NettyAsyncHttpClientBuilder(); if (clientOptions != null) { - if (clientOptions.getConfiguration() != null) { - DEFAULT_HTTP_CLIENT.compareAndSet(null, new NettyAsyncHttpClientBuilder().build()); - return DEFAULT_HTTP_CLIENT.get(); - } builder = builder.proxy(clientOptions.getProxyOptions()) .configuration(clientOptions.getConfiguration()) diff --git a/sdk/core/azure-core-http-netty/src/test/java/com/azure/core/http/netty/NettyAsyncHttpClientTests.java b/sdk/core/azure-core-http-netty/src/test/java/com/azure/core/http/netty/NettyAsyncHttpClientTests.java index 3651b7c5e3e8..38c44129f4b5 100644 --- a/sdk/core/azure-core-http-netty/src/test/java/com/azure/core/http/netty/NettyAsyncHttpClientTests.java +++ b/sdk/core/azure-core-http-netty/src/test/java/com/azure/core/http/netty/NettyAsyncHttpClientTests.java @@ -18,6 +18,7 @@ import com.azure.core.http.policy.RetryPolicy; import com.azure.core.util.Context; import com.azure.core.util.FluxUtil; +import com.azure.core.util.HttpClientOptions; import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.http.Fault; import io.netty.handler.proxy.ProxyConnectException; @@ -58,6 +59,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertLinesMatch; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; public class NettyAsyncHttpClientTests { @@ -459,6 +461,21 @@ public void failedProxyAuthenticationReturnsCorrectError() { } } + @Test + public void testSingletonClientInstanceCreation() { + HttpClient client1 = new NettyAsyncHttpClientProvider().createInstance(); + HttpClient client2 = new NettyAsyncHttpClientProvider().createInstance(); + assertEquals(client1, client2); + } + + @Test + public void testCustomizedClientInstanceCreationNotShared() { + HttpClientOptions clientOptions = new HttpClientOptions().setMaximumConnectionPoolSize(500); + HttpClient client1 = new NettyAsyncHttpClientProvider().createInstance(clientOptions); + HttpClient client2 = new NettyAsyncHttpClientProvider().createInstance(clientOptions); + assertNotEquals(client1, client2); + } + private static Stream requestHeaderSupplier() { return Stream.of( Arguments.of(null, NettyAsyncHttpClientResponseTransformer.NULL_REPLACEMENT), diff --git a/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java b/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java index 248050f101f0..449045631243 100644 --- a/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java +++ b/sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java @@ -19,10 +19,9 @@ public final class OkHttpAsyncClientProvider implements HttpClientProvider { @Override public HttpClient createInstance() { - HttpClient httpClient = new OkHttpAsyncHttpClientBuilder().build(); + // by default use a singleton instance of http client DEFAULT_HTTP_CLIENT.compareAndSet(null, new OkHttpAsyncHttpClientBuilder().build()); - // by default use a singleton http client - return httpClient; + return DEFAULT_HTTP_CLIENT.get(); } @Override @@ -30,19 +29,15 @@ public HttpClient createInstance(HttpClientOptions clientOptions) { OkHttpAsyncHttpClientBuilder builder = new OkHttpAsyncHttpClientBuilder(); if (clientOptions != null) { - if (clientOptions.getConfiguration() != null) { - DEFAULT_HTTP_CLIENT.compareAndSet(null, new OkHttpAsyncHttpClientBuilder().build()); - return DEFAULT_HTTP_CLIENT.get(); - } - builder = builder.proxy(clientOptions.getProxyOptions()) .configuration(clientOptions.getConfiguration()) .writeTimeout(clientOptions.getWriteTimeout()) .readTimeout(clientOptions.getReadTimeout()); - int maximumConnectionPoolSize = (clientOptions.getMaximumConnectionPoolSize() == 0) - ? 5 // By default OkHttp uses a maximum idle connection count of 5. - : clientOptions.getMaximumConnectionPoolSize(); + Integer poolSize = clientOptions.getMaximumConnectionPoolSize(); + int maximumConnectionPoolSize = (poolSize != null && poolSize > 0) + ? poolSize + : 5; // By default, OkHttp uses a maximum idle connection count of 5. ConnectionPool connectionPool = new ConnectionPool(maximumConnectionPoolSize, clientOptions.getConnectionIdleTimeout().toMillis(), TimeUnit.MILLISECONDS); diff --git a/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientTests.java b/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientTests.java index 09a16a9fae57..fc54c667db00 100644 --- a/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientTests.java +++ b/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientTests.java @@ -9,6 +9,7 @@ import com.azure.core.http.HttpMethod; import com.azure.core.http.HttpRequest; import com.azure.core.http.HttpResponse; +import com.azure.core.util.HttpClientOptions; import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.core.WireMockConfiguration; import org.junit.jupiter.api.AfterAll; @@ -42,6 +43,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertLinesMatch; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; public class OkHttpAsyncHttpClientTests { @@ -271,6 +273,21 @@ public void validateHeadersReturnAsIs() { .verify(Duration.ofSeconds(10)); } + @Test + public void testSingletonClientInstanceCreation() { + HttpClient client1 = new OkHttpAsyncClientProvider().createInstance(); + HttpClient client2 = new OkHttpAsyncClientProvider().createInstance(); + assertEquals(client1, client2); + } + + @Test + public void testCustomizedClientInstanceCreationNotShared() { + HttpClientOptions clientOptions = new HttpClientOptions().setMaximumConnectionPoolSize(500); + HttpClient client1 = new OkHttpAsyncClientProvider().createInstance(clientOptions); + HttpClient client2 = new OkHttpAsyncClientProvider().createInstance(clientOptions); + assertNotEquals(client1, client2); + } + private static MessageDigest md5Digest() { try { return MessageDigest.getInstance("MD5"); diff --git a/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java b/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java index 668f8ad26e19..29de34d5a17c 100644 --- a/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java +++ b/sdk/core/azure-core/src/main/java/com/azure/core/util/Configuration.java @@ -133,11 +133,6 @@ public class Configuration implements Cloneable { */ public static final String PROPERTY_AZURE_TRACING_DISABLED = "AZURE_TRACING_DISABLED"; - /** - * Flag to disable sharing of http clients if not configured by the user. - */ - public static final String PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED = "PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED"; - /* * Configurations that are loaded into the global configuration store when the application starts. */ @@ -164,8 +159,7 @@ public class Configuration implements Cloneable { PROPERTY_AZURE_HTTP_LOG_DETAIL_LEVEL, PROPERTY_AZURE_TRACING_DISABLED, PROPERTY_AZURE_POD_IDENTITY_TOKEN_URL, - PROPERTY_AZURE_REGIONAL_AUTHORITY_NAME, - PROPERTY_HTTP_CLIENT_SINGLETON_DISABLED + PROPERTY_AZURE_REGIONAL_AUTHORITY_NAME }; /* From 3b892317ee54671934d995e841a74df6bdacbc7a Mon Sep 17 00:00:00 2001 From: samvaity Date: Mon, 25 Oct 2021 16:37:36 -0700 Subject: [PATCH 3/3] clear OkHttpAsyncHttpClientBuilderTests.java changes --- .../OkHttpAsyncHttpClientBuilderTests.java | 33 ------------------- 1 file changed, 33 deletions(-) diff --git a/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientBuilderTests.java b/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientBuilderTests.java index 8b2efedea034..3afd5e77e2f4 100644 --- a/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientBuilderTests.java +++ b/sdk/core/azure-core-http-okhttp/src/test/java/com/azure/core/http/okhttp/OkHttpAsyncHttpClientBuilderTests.java @@ -378,27 +378,6 @@ public void buildWithConfigurationNone() { .verifyComplete(); } - @Test - public void testToDetermineWherePrintHappens() { - OkHttpClient validatorClient = okHttpClientWithProxyValidation(false, Proxy.Type.HTTP); - - String rawJavaNonProxyHosts = String.join("|", "localhost", "127.0.0.1", "*.microsoft.com", "*.linkedin.com"); - - Configuration configuration = new Configuration() - .put(JAVA_PROXY_PREREQUISITE, "true") - .put(JAVA_HTTP_PROXY_HOST, "localhost") - .put(JAVA_HTTP_PROXY_PORT, "12345") - .put(JAVA_NON_PROXY_HOSTS, rawJavaNonProxyHosts); - - HttpClient okClient = new OkHttpAsyncHttpClientBuilder(validatorClient) - .configuration(configuration) - .build(); - - StepVerifier.create(okClient.send(new HttpRequest(HttpMethod.GET, "http://localhost"))) - .verifyErrorMatches(throwable -> throwable.getMessage() - .contains(TestEventListenerValidator.EXPECTED_EXCEPTION_MESSAGE)); - } - @ParameterizedTest @MethodSource("buildWithConfigurationProxySupplier") public void buildWithConfigurationProxy(boolean shouldHaveProxy, Configuration configuration, String requestUrl) { @@ -507,18 +486,6 @@ private static Stream buildWithConfigurationProxySupplier() { private static OkHttpClient okHttpClientWithProxyValidation(boolean shouldHaveProxy, Proxy.Type proxyType) { return new OkHttpClient.Builder() .eventListener(new TestEventListenerValidator(shouldHaveProxy, proxyType)) - // Use a custom Dispatcher and ExecutorService which overrides the uncaught exception handler. - // This is done to prevent the tests using this from printing their error stack trace. - // The reason this happens is the test throws an exception which goes uncaught in a thread, and this is an - // expected exception, which results in the exception and its stack trace being logged, which is very - // verbose. - .dispatcher(new Dispatcher(Executors.newFixedThreadPool(2, r -> { - Thread thread = new Thread(r); - thread.setUncaughtExceptionHandler((t, e) -> { - }); - - return thread; - }))) .build(); }