spring/spring-cloud-netflix
1
0
Fork 0
mirror of https://github.com/spring-cloud/spring-cloud-netflix.git synced 2026-09-17 07:43:44 +00:00
Code Issues Projects Releases Wiki Activity

Move shared HTTP client caching from supplier to transport factory

Browse Source 
Signed-off-by: Prahlad Bhakat <prahladbhakat05@gmail.com>
This commit is contained in:
Prahlad Bhakat
2026-09-16 11:25:24 -04:00
committed by Ryan Baxter
parent 5a1391f196
commit 9ebf9cff58
5 changed files with 121 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplier.java
+14 -33
View File
@@ -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,10 +61,6 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie
private final Set<RequestConfigCustomizer> requestConfigCustomizers;
private volatile CloseableHttpClient sharedHttpClient;
private final Object lock = new Object();
public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties,
Set<RequestConfigCustomizer> requestConfigCustomizers) {
this.timeoutProperties = timeoutProperties;
@@ -67,40 +69,19 @@ 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;
}
}
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 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
spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/EurekaClientHttpRequestFactorySupplier.java
-11
View File
@@ -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.
*
spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactory.java
+34 -4
View File
@@ -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) {
spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplierTests.java
+16 -33
View File
@@ -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);
}
}
spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java
+57 -19
View File
@@ -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();
}
}