Merge pull request #3 from samvaity/share-http-clients

Use singleton instance creation for HttpClientProviders

sdk/core/azure-core-http-jdk-httpclient/src/main/java/com/azure/core/http/jdk/httpclient/JdkHttpClientProvider.java

@@ -19,6 +19,7 @@ public final class JdkHttpClientProvider implements HttpClientProvider {
 
     @Override
     public HttpClient createInstance() {
+        // by default use a singleton instance of http client
         DEFAULT_HTTP_CLIENT.compareAndSet(null, new JdkAsyncHttpClientBuilder().build());
 
         return DEFAULT_HTTP_CLIENT.get();

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");

sdk/core/azure-core-http-netty/src/main/java/com/azure/core/http/netty/NettyAsyncHttpClientProvider.java

@@ -18,8 +18,8 @@ public final class NettyAsyncHttpClientProvider implements HttpClientProvider {
 
     @Override
     public HttpClient createInstance() {
+        // by default use a singleton instance of http client
         DEFAULT_HTTP_CLIENT.compareAndSet(null, new NettyAsyncHttpClientBuilder().build());
-
         return DEFAULT_HTTP_CLIENT.get();
     }
 
@@ -28,6 +28,7 @@ public HttpClient createInstance(HttpClientOptions clientOptions) {
         NettyAsyncHttpClientBuilder builder = new NettyAsyncHttpClientBuilder();
 
         if (clientOptions != null) {
+
             builder = builder.proxy(clientOptions.getProxyOptions())
                 .configuration(clientOptions.getConfiguration())
                 .writeTimeout(clientOptions.getWriteTimeout())

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<Arguments> requestHeaderSupplier() {
         return Stream.of(
             Arguments.of(null, NettyAsyncHttpClientResponseTransformer.NULL_REPLACEMENT),

sdk/core/azure-core-http-okhttp/src/main/java/com/azure/core/http/okhttp/OkHttpAsyncClientProvider.java

@@ -19,8 +19,8 @@ public final class OkHttpAsyncClientProvider implements HttpClientProvider {
 
     @Override
     public HttpClient createInstance() {
+        // by default use a singleton instance of http client
         DEFAULT_HTTP_CLIENT.compareAndSet(null, new OkHttpAsyncHttpClientBuilder().build());
-
         return DEFAULT_HTTP_CLIENT.get();
     }
 
@@ -34,9 +34,10 @@ public HttpClient createInstance(HttpClientOptions clientOptions) {
                 .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);

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<Arguments> 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();
     }
 

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");