Merge pull request #4585 from PRAHLAD09-dev/fix/4569-eureka-deregistration-shutdown

gh-4569: Deterministically close shared HTTP client on TransportClientFactory#shutdown()

spring-cloud-netflix-eureka-client/src/main/java/org/springframework/cloud/netflix/eureka/http/DefaultEurekaClientHttpRequestFactorySupplier.java

@@ -34,6 +34,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;
@@ -42,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
@@ -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;

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.
  *
+ * <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,8 +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() {
+	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

@@ -0,0 +1,75 @@
+/*
+ * 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}.
+ *
+ * <p>
+ * 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);
+	}
+
+}

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