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

@@ -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.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,27 +55,47 @@ public class DefaultEurekaClientHttpRequestFactorySupplier implements EurekaClie
 
 	private final Set<RequestConfigCustomizer> requestConfigCustomizers;
 
+	private CloseableHttpClient sharedHttpClient;
+
 	public DefaultEurekaClientHttpRequestFactorySupplier(TimeoutProperties timeoutProperties,
 			Set<RequestConfigCustomizer> requestConfigCustomizers) {
 		this.timeoutProperties = timeoutProperties;
 		this.requestConfigCustomizers = requestConfigCustomizers;
 	}
 
 	@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));
+	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;
 		}
-		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

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.
 	 *

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

@@ -107,6 +107,7 @@ public EurekaHttpClient newClient(EurekaEndpoint endpoint) {
 
 	@Override
 	public void shutdown() {
+		eurekaClientHttpRequestFactorySupplier.close();
 	}
 
 	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,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}.
+ *
+ * <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; 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();
+	}
+
+}

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