Move shared HTTP client caching from supplier to transport factory

Signed-off-by: Prahlad Bhakat <prahladbhakat05@gmail.com>
2026-09-17 15:49:00 +00:00 · 2026-09-09 15:25:05 +05:30
parent 8a1569162b
commit cc51cd8735
5 changed files with 122 additions and 96 deletions
@@ -16,7 +16,6 @@

 package org.springframework.cloud.netflix.eureka.http;

-import java.io.IOException;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;

@@ -44,6 +43,13 @@ import org.springframework.lang.Nullable;
 * Supplier for the {@link ClientHttpRequestFactory} to be used by Eureka client that uses
 * {@link HttpClients}.
 *
+ * <p>
+ * This supplier is intentionally stateless: each call to {@link #get} builds a fresh
+ * {@link CloseableHttpClient}. Caching and lifecycle management of the shared client is
+ * the responsibility of the owning {@code TransportClientFactory}
+ * ({@link RestClientTransportClientFactory}), which is already created once per Eureka
+ * client and is the natural owner of that client's lifecycle.
+ *
 * @author Marcin Grzejszczak
 * @author Olga Maciaszek-Sharma
 * @author Jiwon Jeon
@@ -55,8 +61,6 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie

 	private final Set<RequestConfigCustomizer> requestConfigCustomizers;

-	private CloseableHttpClient sharedHttpClient;
-
 	public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties,
 			Set<RequestConfigCustomizer> requestConfigCustomizers) {
 		this.timeoutProperties = timeoutProperties;
@@ -64,38 +68,20 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie
 	}

 	@Override
-	public synchronized ClientHttpRequestFactory get(SSLContext sslContext,
-			@Nullable HostnameVerifier hostnameVerifier) {
-		CloseableHttpClient httpClient = this.sharedHttpClient;
-		if (httpClient == null) {
+	public ClientHttpRequestFactory get(SSLContext sslContext, @Nullable HostnameVerifier hostnameVerifier) {
 		HttpClientBuilder httpClientBuilder = HttpClientBuilder.create();
 		if (sslContext != null || hostnameVerifier != null || timeoutProperties != null) {
 			httpClientBuilder
 				.setConnectionManager(buildConnectionManager(sslContext, hostnameVerifier, timeoutProperties));
 		}
 		httpClientBuilder.setDefaultRequestConfig(buildRequestConfig());
-			httpClient = httpClientBuilder.build();
-			this.sharedHttpClient = httpClient;
-		}
+		CloseableHttpClient httpClient = httpClientBuilder.build();
+
 		HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory();
 		requestFactory.setHttpClient(httpClient);
 		return requestFactory;
 	}

-	@Override
-	public synchronized void close() {
-		CloseableHttpClient httpClient = this.sharedHttpClient;
-		this.sharedHttpClient = null;
-		if (httpClient != null) {
-			try {
-				httpClient.close();
-			}
-			catch (IOException ex) {
-				// best-effort close during shutdown; nothing actionable if it fails
-			}
-		}
-	}
-
 	private HttpClientConnectionManager buildConnectionManager(SSLContext sslContext, HostnameVerifier hostnameVerifier,
 			TimeoutProperties timeoutProperties) {
 		PoolingHttpClientConnectionManagerBuilder connectionManagerBuilder = PoolingHttpClientConnectionManagerBuilder
@@ -40,17 +40,6 @@ public interface EurekaClientHttpRequestFactorySupplier {
 	 */
 	ClientHttpRequestFactory get(SSLContext sslContext, @Nullable HostnameVerifier hostnameVerifier);

-	/**
-	 * Closes any resources (e.g. a shared HTTP client / connection pool) held by this
-	 * supplier. Called by the owning
-	 * {@link com.netflix.discovery.shared.transport.TransportClientFactory} on
-	 * {@code shutdown()}, which Netflix's {@code DiscoveryClient} invokes synchronously,
-	 * right after the final {@code unregister()} call completes.
-	 * @since 4.3.0
-	 */
-	default void close() {
-	}
-
 	/**
 	 * Allows customising the {@link RequestConfig} of the underlying Apache HC5 instance.
 	 *
@@ -16,6 +16,8 @@

 package org.springframework.cloud.netflix.eureka.http;

+import java.io.Closeable;
+import java.io.IOException;
 import java.util.Optional;
 import java.util.function.Supplier;

@@ -31,6 +33,7 @@ import org.springframework.http.HttpStatus;
 import org.springframework.http.HttpStatusCode;
 import org.springframework.http.client.ClientHttpRequestFactory;
 import org.springframework.http.client.ClientHttpResponse;
+import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;
 import org.springframework.http.client.support.BasicAuthenticationInterceptor;
 import org.springframework.web.client.RestClient;
 import org.springframework.web.util.UriComponentsBuilder;
@@ -44,6 +47,15 @@ import static org.springframework.cloud.netflix.eureka.http.EurekaHttpClientUtil
 * {@link RestClientEurekaHttpClient}. Relies on Jackson for serialization and
 * deserialization.
 *
+ * <p>
+ * A single {@link ClientHttpRequestFactory} (and the underlying HTTP client it wraps) is
+ * lazily built on the first call to {@link #newClient} and reused for every subsequent
+ * call on this factory instance. Since each {@code RestClientTransportClientFactory} is
+ * already scoped to a single Eureka client, caching the request factory here - rather
+ * than in a potentially shared {@link EurekaClientHttpRequestFactorySupplier} - ties the
+ * shared client's lifecycle directly to this factory instance, so {@link #shutdown()}
+ * only ever closes a client owned exclusively by this factory.
+ *
 * @author Wonchul Heo
 * @author Olga Maciaszek-Sharma
 * @since 4.2.0
@@ -58,6 +70,8 @@ public class RestClientTransportClientFactory implements TransportClientFactory

 	private final Supplier<RestClient.Builder> builderSupplier;

+	private ClientHttpRequestFactory cachedRequestFactory;
+
 	public RestClientTransportClientFactory(Optional<SSLContext> sslContext,
 			Optional<HostnameVerifier> hostnameVerifier,
 			EurekaClientHttpRequestFactorySupplier eurekaClientHttpRequestFactorySupplier,
@@ -84,8 +98,7 @@ public class RestClientTransportClientFactory implements TransportClientFactory
 		// we want a copy to modify. Don't change the original
 		final RestClient.Builder builder = builderSupplier.get().clone();

-		ClientHttpRequestFactory requestFactory = this.eurekaClientHttpRequestFactorySupplier
-			.get(this.sslContext.orElse(null), this.hostnameVerifier.orElse(null));
+		ClientHttpRequestFactory requestFactory = getOrCreateRequestFactory();

 		builder.requestFactory(requestFactory);
 		setUrl(builder, endpoint.getServiceUrl());
@@ -105,9 +118,26 @@ public class RestClientTransportClientFactory implements TransportClientFactory
 		return new RestClientEurekaHttpClient(builder.build());
 	}

+	private synchronized ClientHttpRequestFactory getOrCreateRequestFactory() {
+		if (this.cachedRequestFactory == null) {
+			this.cachedRequestFactory = this.eurekaClientHttpRequestFactorySupplier.get(this.sslContext.orElse(null),
+					this.hostnameVerifier.orElse(null));
+		}
+		return this.cachedRequestFactory;
+	}
+
 	@Override
-	public void shutdown() {
-		eurekaClientHttpRequestFactorySupplier.close();
+	public synchronized void shutdown() {
+		if (this.cachedRequestFactory instanceof HttpComponentsClientHttpRequestFactory httpRequestFactory
+				&& httpRequestFactory.getHttpClient() instanceof Closeable closeableHttpClient) {
+			try {
+				closeableHttpClient.close();
+			}
+			catch (IOException ex) {
+				// best-effort close during shutdown; nothing actionable if it fails
+			}
+		}
+		this.cachedRequestFactory = null;
 	}

 	private static void setUrl(RestClient.Builder builder, String serviceUrl) {
@@ -34,10 +34,11 @@ import static org.assertj.core.api.Assertions.assertThat;
 * These specifically guard against regressing gh-4275: an earlier fix (gh-4258) made this
 * class a Spring {@code DisposableBean}, which raced with
 * {@code CloudEurekaClient#shutdown()} during context shutdown and broke
- * unregister-on-shutdown. That fix was reverted; this class must continue to be closed
- * only via {@link EurekaClientHttpRequestFactorySupplier#close()}, invoked synchronously
- * by {@code TransportClientFactory#shutdown()} - never via an independent Spring
- * bean-destroy callback.
+ * unregister-on-shutdown. That fix was reverted, and this supplier is now intentionally
+ * stateless (gh-4569): it never caches or closes an HTTP client itself. Lifecycle
+ * management of the shared client belongs to the owning
+ * {@link RestClientTransportClientFactory}, which is already scoped to a single Eureka
+ * client - see {@link RestClientTransportClientFactoryShutdownTests}.
 */
 class DefaultEurekaClientHttpRequestFactorySupplierTests {

@@ -52,41 +53,23 @@ class DefaultEurekaClientHttpRequestFactorySupplierTests {
 	}

 	@Test
-	void shouldReuseSameHttpClientAcrossMultipleGetCalls() {
+	void getShouldReturnANonNullRequestFactory() {
+		ClientHttpRequestFactory requestFactory = supplier.get(null, null);
+		assertThat(requestFactory).isNotNull();
+	}
+
+	@Test
+	void getShouldBuildAFreshHttpClientOnEveryCall() {
+		// The supplier is stateless - caching and lifecycle management belong to the
+		// caller (RestClientTransportClientFactory), so every call must return an
+		// independent client rather than a shared one.
 		ClientHttpRequestFactory first = supplier.get(null, null);
 		ClientHttpRequestFactory second = supplier.get(null, null);

 		Object firstHttpClient = ((HttpComponentsClientHttpRequestFactory) first).getHttpClient();
 		Object secondHttpClient = ((HttpComponentsClientHttpRequestFactory) second).getHttpClient();

-		assertThat(firstHttpClient).isSameAs(secondHttpClient);
-	}
-
-	@Test
-	void closeShouldBeSafeToCallWithoutPriorGet() {
-		// close() before get() (e.g. context shut down before any request was ever
-		// made) must not throw.
-		supplier.close();
-	}
-
-	@Test
-	void closeShouldBeSafeToCallTwice() {
-		supplier.get(null, null);
-		supplier.close();
-		// Idempotent - shutdown paths may call close() more than once.
-		supplier.close();
-	}
-
-	@Test
-	void getAfterCloseShouldStillReturnARequestFactory() {
-		supplier.get(null, null);
-		supplier.close();
-
-		// A get() call racing just after shutdown must not throw; the returned factory
-		// wraps a closed client and will fail on actual use, which is expected during
-		// shutdown, but construction itself must remain safe.
-		ClientHttpRequestFactory afterClose = supplier.get(null, null);
-		assertThat(afterClose).isNotNull();
+		assertThat(firstHttpClient).isNotSameAs(secondHttpClient);
 	}

 }
@@ -16,42 +16,80 @@

 package org.springframework.cloud.netflix.eureka.http;

+import java.util.Collections;
+
 import org.junit.jupiter.api.Test;

 import org.springframework.cloud.configuration.TlsProperties;
+import org.springframework.cloud.netflix.eureka.TimeoutProperties;
+import org.springframework.http.client.HttpComponentsClientHttpRequestFactory;

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;

 /**
- * Tests that {@link RestClientTransportClientFactory#shutdown()} deterministically
- * delegates to {@link EurekaClientHttpRequestFactorySupplier#close()}, closing the shared
- * HTTP client/pool synchronously - after the caller (Netflix's {@code DiscoveryClient})
- * has already completed its final {@code unregister()} call, and not via a separate,
- * unordered Spring bean-destroy path (gh-4569).
+ * Tests that {@link RestClientTransportClientFactory} owns and deterministically closes
+ * the shared HTTP client it lazily builds via
+ * {@link EurekaClientHttpRequestFactorySupplier#get}, rather than delegating that
+ * lifecycle to a potentially shared supplier instance (gh-4569).
+ *
+ * <p>
+ * Since each {@code RestClientTransportClientFactory} is already scoped to a single
+ * Eureka client, caching the client here means
+ * {@link RestClientTransportClientFactory#shutdown()} only ever closes a client owned
+ * exclusively by this factory - there is no shared-state hazard between a refreshed
+ * client or a second, independently-created Eureka client, as there would be if the
+ * client were cached in the supplier itself.
 */
 class RestClientTransportClientFactoryShutdownTests {

+	private final DefaultEurekaClientHttpRequestFactorySupplier supplier = new DefaultEurekaClientHttpRequestFactorySupplier(
+			new TimeoutProperties(), Collections.emptySet());
+
+	private final RestClientTransportClientFactory factory = new RestClientTransportClientFactory(new TlsProperties(),
+			supplier);
+
 	@Test
-	void shutdownShouldCloseTheHttpRequestFactorySupplier() {
-		EurekaClientHttpRequestFactorySupplier supplier = mock(EurekaClientHttpRequestFactorySupplier.class);
-		RestClientTransportClientFactory factory = new RestClientTransportClientFactory(new TlsProperties(), supplier);
-
-		factory.shutdown();
-
-		verify(supplier, times(1)).close();
+	void shutdownBeforeAnyNewClientCallShouldNotThrow() {
+		// shutdown() before newClient() (e.g. context shut down before any request was
+		// ever made) must not throw.
+		assertThatCode(factory::shutdown).doesNotThrowAnyException();
 	}

 	@Test
 	void shutdownShouldBeIdempotent() {
-		EurekaClientHttpRequestFactorySupplier supplier = mock(EurekaClientHttpRequestFactorySupplier.class);
-		RestClientTransportClientFactory factory = new RestClientTransportClientFactory(new TlsProperties(), supplier);
-
-		factory.shutdown();
+		factory.newClient(endpoint());
 		factory.shutdown();
+		// Idempotent - shutdown paths may call shutdown() more than once.
+		assertThatCode(factory::shutdown).doesNotThrowAnyException();
+	}

-		verify(supplier, times(2)).close();
+	@Test
+	void repeatedNewClientCallsShouldReuseTheSameCachedRequestFactory() {
+		// newClient() delegates to the same cached request factory on every call, rather
+		// than asking the supplier for a fresh one each time.
+		factory.newClient(endpoint());
+		Object firstHttpClient = cachedHttpClient(factory);
+
+		factory.newClient(endpoint());
+		Object secondHttpClient = cachedHttpClient(factory);
+
+		assertThat(firstHttpClient).isSameAs(secondHttpClient);
+	}
+
+	private static com.netflix.discovery.shared.resolver.EurekaEndpoint endpoint() {
+		com.netflix.discovery.shared.resolver.EurekaEndpoint endpoint = mock(
+				com.netflix.discovery.shared.resolver.EurekaEndpoint.class);
+		when(endpoint.getServiceUrl()).thenReturn("http://localhost:8761/eureka/");
+		return endpoint;
+	}
+
+	private static Object cachedHttpClient(RestClientTransportClientFactory factory) {
+		Object requestFactory = org.springframework.test.util.ReflectionTestUtils.getField(factory,
+				"cachedRequestFactory");
+		return ((HttpComponentsClientHttpRequestFactory) requestFactory).getHttpClient();
 	}

 }