+ * 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 @@ -67,8 +75,8 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie .setConnectionManager(buildConnectionManager(sslContext, hostnameVerifier, timeoutProperties)); } httpClientBuilder.setDefaultRequestConfig(buildRequestConfig()); - CloseableHttpClient httpClient = httpClientBuilder.build(); + HttpComponentsClientHttpRequestFactory requestFactory = new HttpComponentsClientHttpRequestFactory(); requestFactory.setHttpClient(httpClient); return requestFactory; 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..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
+ * 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, 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 {
+
+ 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 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).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
new file mode 100644
index 000000000..48dcd2c56
--- /dev/null
+++ b/spring-cloud-netflix-eureka-client/src/test/java/org/springframework/cloud/netflix/eureka/http/RestClientTransportClientFactoryShutdownTests.java
@@ -0,0 +1,111 @@
+/*
+ * 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.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.when;
+
+/**
+ * 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 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() {
+ factory.newClient(endpoint());
+ factory.shutdown();
+ // Idempotent - shutdown paths may call shutdown() more than once.
+ assertThatCode(factory::shutdown).doesNotThrowAnyException();
+ }
+
+ @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();
+ }
+
+ @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);
+ }
+
+}