From f37374d472884c2d7789e82475fc423789a70b67 Mon Sep 17 00:00:00 2001 From: Prahlad Bhakat Date: Fri, 14 Aug 2026 16:33:29 +0530 Subject: [PATCH 1/5] Fix Eureka HTTP client shutdown Signed-off-by: Prahlad Bhakat --- ...urekaClientHttpRequestFactorySupplier.java | 41 +++++++-- ...urekaClientHttpRequestFactorySupplier.java | 11 +++ .../RestClientTransportClientFactory.java | 1 + ...ClientHttpRequestFactorySupplierTests.java | 92 +++++++++++++++++++ ...ntTransportClientFactoryShutdownTests.java | 57 ++++++++++++ 5 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java create mode 100644 spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java 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 3db961365..3e43d883c 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,6 +16,7 @@ package org.springframework.cloud.netflix.eureka.http; +import java.io.IOException; import java.util.Set; import java.util.concurrent.TimeUnit; @@ -34,6 +35,7 @@ import org.apache.hc.core5.http.io.SocketConfig; import org.apache.hc.core5.util.Timeout; import org.springframework.cloud.netflix.eureka.TimeoutProperties; +import org.springframework.cloud.netflix.eureka.http.EurekaClientHttpRequestFactorySupplier.RequestConfigCustomizer; import org.springframework.http.client.ClientHttpRequestFactory; import org.springframework.http.client.HttpComponentsClientHttpRequestFactory; import org.springframework.lang.Nullable; @@ -53,6 +55,10 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie private final Set requestConfigCustomizers; + private volatile CloseableHttpClient sharedHttpClient; + + private final Object lock = new Object(); + public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties, Set requestConfigCustomizers) { this.timeoutProperties = timeoutProperties; @@ -61,19 +67,40 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie @Override 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)); + CloseableHttpClient httpClient = this.sharedHttpClient; + if (httpClient == null) { + synchronized (this.lock) { + 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; + } + } } - httpClientBuilder.setDefaultRequestConfig(buildRequestConfig()); - - CloseableHttpClient httpClient = httpClientBuilder.build(); HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(); requestFactory.setHttpClient(httpClient); return requestFactory; } + @Override + public void close() { + CloseableHttpClient httpClient = this.sharedHttpClient; + 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 9157edcd6..0bc64992f 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,6 +40,17 @@ 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 e09e4b572..1405ab888 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 @@ -107,6 +107,7 @@ public class RestClientTransportClientFactory implements TransportClientFactory @Override public void shutdown() { + eurekaClientHttpRequestFactorySupplier.close(); } 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 new file mode 100644 index 000000000..2903986f4 --- /dev/null +++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java @@ -0,0 +1,92 @@ +/* + * Copyright 2013-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.netflix.eureka.http; + +import java.util.Collections; + +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.DisposableBean; +import org.springframework.cloud.netflix.eureka.TimeoutProperties; +import org.springframework.http.client.ClientHttpRequestFactory; +import org.springframework.http.client.HttpComponentsClientHttpRequestFactory; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link DefaultEurekaClientHttpRequestFactorySupplier}. + * + *

+ * 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. + */ +class DefaultEurekaClientHttpRequestFactorySupplierTests { + + private final DefaultEurekaClientHttpRequestFactorySupplier supplier = new DefaultEurekaClientHttpRequestFactorySupplier( + new TimeoutProperties(), Collections.emptySet()); + + @Test + void shouldNotBeADisposableBean() { + // Guard against reintroducing gh-4275: this class must not be destroyed via an + // independent Spring bean-destroy callback. + assertThat(supplier).isNotInstanceOf(DisposableBean.class); + } + + @Test + void shouldReuseSameHttpClientAcrossMultipleGetCalls() { + 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(); + } + +} 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 new file mode 100644 index 000000000..aeb8ef227 --- /dev/null +++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java @@ -0,0 +1,57 @@ +/* + * Copyright 2013-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.netflix.eureka.http; + +import org.junit.jupiter.api.Test; + +import org.springframework.cloud.configuration.TlsProperties; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +/** + * 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). + */ +class RestClientTransportClientFactoryShutdownTests { + + @Test + void shutdownShouldCloseTheHttpRequestFactorySupplier() { + EurekaClientHttpRequestFactorySupplier supplier = mock(EurekaClientHttpRequestFactorySupplier.class); + RestClientTransportClientFactory factory = new RestClientTransportClientFactory(new TlsProperties(), supplier); + + factory.shutdown(); + + verify(supplier, times(1)).close(); + } + + @Test + void shutdownShouldBeIdempotent() { + EurekaClientHttpRequestFactorySupplier supplier = mock(EurekaClientHttpRequestFactorySupplier.class); + RestClientTransportClientFactory factory = new RestClientTransportClientFactory(new TlsProperties(), supplier); + + factory.shutdown(); + factory.shutdown(); + + verify(supplier, times(2)).close(); + } + +} From b2d3f8225b492dfa931b8557cbca824e813a7a16 Mon Sep 17 00:00:00 2001 From: Prahlad Bhakat Date: Wed, 26 Aug 2026 13:10:13 +0530 Subject: [PATCH 2/5] Fix Eureka HTTP client lifecycle Signed-off-by: Prahlad Bhakat --- ...urekaClientHttpRequestFactorySupplier.java | 45 ++++++++++--------- 1 file changed, 23 insertions(+), 22 deletions(-) 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 3e43d883c..7c6ab8435 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 @@ -55,10 +55,10 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie private final Set requestConfigCustomizers; - private volatile CloseableHttpClient sharedHttpClient; - private final Object lock = new Object(); + private volatile CloseableHttpClient sharedHttpClient; + public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties, Set requestConfigCustomizers) { this.timeoutProperties = timeoutProperties; @@ -67,20 +67,18 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie @Override public ClientHttpRequestFactory get(SSLContext sslContext, @Nullable HostnameVerifier hostnameVerifier) { - CloseableHttpClient httpClient = this.sharedHttpClient; - if (httpClient == null) { - synchronized (this.lock) { - 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; + CloseableHttpClient httpClient; + synchronized (this.lock) { + 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; } } HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(); @@ -90,13 +88,16 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie @Override public void close() { - CloseableHttpClient httpClient = this.sharedHttpClient; - if (httpClient != null) { - try { - httpClient.close(); - } - catch (IOException ex) { - // best-effort close during shutdown; nothing actionable if it fails + synchronized (this.lock) { + 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 + } } } } From 8a1569162b346972b2b185718303d4193cd9f61c Mon Sep 17 00:00:00 2001 From: Prahlad Bhakat Date: Thu, 27 Aug 2026 20:36:40 +0530 Subject: [PATCH 3/5] Simplify Eureka HTTP client synchronization Signed-off-by: Prahlad Bhakat --- ...urekaClientHttpRequestFactorySupplier.java | 48 ++++++++----------- 1 file changed, 21 insertions(+), 27 deletions(-) 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 7c6ab8435..24cd52265 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 @@ -55,9 +55,7 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie private final Set requestConfigCustomizers; - private final Object lock = new Object(); - - private volatile CloseableHttpClient sharedHttpClient; + private CloseableHttpClient sharedHttpClient; public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties, Set requestConfigCustomizers) { @@ -66,20 +64,18 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie } @Override - public ClientHttpRequestFactory get(SSLContext sslContext, @Nullable HostnameVerifier hostnameVerifier) { - CloseableHttpClient httpClient; - synchronized (this.lock) { - 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 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; } HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(); requestFactory.setHttpClient(httpClient); @@ -87,17 +83,15 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie } @Override - public void close() { - synchronized (this.lock) { - 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 - } + 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 } } } From cc51cd87359bb38b717b7eee73b22ec2ddd6453e Mon Sep 17 00:00:00 2001 From: Prahlad Bhakat Date: Wed, 9 Sep 2026 15:23:21 +0530 Subject: [PATCH 4/5] Move shared HTTP client caching from supplier to transport factory Signed-off-by: Prahlad Bhakat --- ...urekaClientHttpRequestFactorySupplier.java | 44 ++++------- ...urekaClientHttpRequestFactorySupplier.java | 11 --- .../RestClientTransportClientFactory.java | 38 +++++++++- ...ClientHttpRequestFactorySupplierTests.java | 49 ++++-------- ...ntTransportClientFactoryShutdownTests.java | 76 ++++++++++++++----- 5 files changed, 122 insertions(+), 96 deletions(-) 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(); } } From 6cc39452dc5714af1fcdca56ab707964e03da7e3 Mon Sep 17 00:00:00 2001 From: Prahlad Bhakat Date: Thu, 10 Sep 2026 18:37:02 +0530 Subject: [PATCH 5/5] Add test verifying newClient() rebuilds after shutdown Signed-off-by: Prahlad Bhakat --- ...lientTransportClientFactoryShutdownTests.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 c2f645de9..48dcd2c56 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 @@ -92,4 +92,20 @@ class RestClientTransportClientFactoryShutdownTests { return ((HttpComponentsClientHttpRequestFactory) requestFactory).getHttpClient(); } + @Test + void newClientAfterShutdownShouldCreateANewCachedRequestFactory() { + factory.newClient(endpoint()); + Object httpClientBeforeShutdown = cachedHttpClient(factory); + + factory.shutdown(); + + factory.newClient(endpoint()); + Object httpClientAfterShutdown = cachedHttpClient(factory); + + // shutdown() clears the cached request factory, so a subsequent newClient() + // call must lazily build a fresh one rather than reusing the client that was + // just closed. + assertThat(httpClientAfterShutdown).isNotSameAs(httpClientBeforeShutdown); + } + }