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