diff --git a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplier.java b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplier.java index 24cd52265..537edd5d1 100644 --- a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplier.java +++ b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplier.java @@ -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}. * + *

+ * 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 requestConfigCustomizers; - private CloseableHttpClient sharedHttpClient; - public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties, Set 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) { - 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; + 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()); + 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 diff --git a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaClientHttpRequestFactorySupplier.java b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaClientHttpRequestFactorySupplier.java index 0bc64992f..9157edcd6 100644 --- a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaClientHttpRequestFactorySupplier.java +++ b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaClientHttpRequestFactorySupplier.java @@ -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. * diff --git a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java index 1405ab888..e4d084e27 100644 --- a/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java +++ b/spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java @@ -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. * + *

+ * 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 builderSupplier; + private ClientHttpRequestFactory cachedRequestFactory; + public RestClientTransportClientFactory(Optional sslContext, Optional 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) { diff --git a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java index 2903986f4..359570313 100644 --- a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java +++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java @@ -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); } } diff --git a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java index aeb8ef227..c2f645de9 100644 --- a/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java +++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java @@ -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). + * + *

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