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

[PRAHLAD09-dev]

Fixes #4569

Problem

RestClientTransportClientFactory.newClient() builds a brand-new CloseableHttpClient (with its own dedicated connection pool) on every single call. Neither TransportClientFactory#shutdown() nor EurekaHttpClient#shutdown() ever closes these clients - cleanup relied entirely on GC. During graceful shutdown, the final unregister() DELETE call (made from DiscoveryClient.shutdown(), right after cancelScheduledTasks()) can hit a connection pool that's already been torn down, throwing:

java.lang.IllegalStateException: Connection pool shut down

This causes deregistration to fail silently during graceful shutdown, leaving stale instances in the Eureka registry until the lease expires.

History

This overlaps with #4103, which documented the same "new HTTP client per call, never closed" issue. That was addressed in #4258 by caching a shared CloseableHttpClient in DefaultEurekaClientHttpRequestFactorySupplier and making the supplier a Spring DisposableBean to close it on shutdown. That approach was reverted (79a2eb8) after it caused #4275: making the supplier a DisposableBean introduced an independent Spring bean-destroy callback with no ordering guarantee relative to CloudEurekaClient's own @Bean(destroyMethod = "shutdown") - so the shared client could be closed before CloudEurekaClient.shutdown() reached its unregister() call, breaking deregistration outright.

Fix

Same idea as #4258 - share one CloseableHttpClient per supplier instance instead of building one per call - but close it through the Eureka transport lifecycle instead of an independent Spring bean-destroy callback:

• EurekaClientHttpRequestFactorySupplier gets a new default void close() {} method (backward compatible for existing custom implementations).
• DefaultEurekaClientHttpRequestFactorySupplier lazily builds and caches a single CloseableHttpClient, reused across all get() calls, and closes it in close(). It does not implement DisposableBean.
• RestClientTransportClientFactory#shutdown() now calls eurekaClientHttpRequestFactorySupplier.close().

Since TransportClientFactory#shutdown() is invoked synchronously by Netflix's DiscoveryClient.shutdown() - which calls unregister() before eurekaTransport.shutdown() - the pool is now guaranteed to close after the final deregistration request completes, in the same thread, with no race against an unrelated Spring bean-destroy path.

Testing

• DefaultEurekaClientHttpRequestFactorySupplierTests: verifies the client is reused across get() calls, close() is safe to call before any get() and safe to call twice, and - as an explicit regression guard for When shutting down after 4.1.1, an exception occurs while unregistering #4275 - that this class does not implement DisposableBean.
• RestClientTransportClientFactoryShutdownTests: verifies shutdown() delegates to supplier.close().
• Full spring-cloud-netflix-eureka-client test suite passes locally (216/216, excluding one pre-existing Docker-dependent Testcontainers test unrelated to this change).

[ryanjbaxter]

I am a little concerned about this in 2 cases:

First the refresh case when eureka.client.refresh.enable=true. A refresh would close his but never nulls it out.

Second when if a second Eureka client is created, then it would reuse this sharedHttpClient, if one calls close if destroys the second instance.

[PRAHLAD09-dev]

Good catch. The refresh case is now handled by clearing sharedHttpClient after closing it, so a subsequent get() creates a fresh client instead of reusing the closed instance.

For the second scenario, where multiple Eureka clients might share the same supplier instance, I wanted to confirm whether that can actually happen with the current RestClientTransportClientFactory lifecycle before introducing additional reference-counting/lifecycle management.

[ryanjbaxter]

For the second scenario, where multiple Eureka clients might share the same supplier instance, I wanted to confirm whether that can actually happen with the current RestClientTransportClientFactory lifecycle before introducing additional reference-counting/lifecycle management.

It is not something we do out of the box, but in theory someone could create a second client on their own and that case they would run into a problem if one of those clients gets closed while the other one is still being used.

[ryanjbaxter]

This is still a concern of mine

[PRAHLAD09-dev]

Thanks for clarifying. I understand the concern around a supplier instance being shared by multiple Eureka clients. Since this is not the default lifecycle but is still a possible use case, would you prefer us to handle shared supplier instances explicitly, for example with reference counting, or document this as a limitation?

[ryanjbaxter]

I don't think this is true anymore since the shared client is nulled

[wind57]

what is the point to make it volatile here? you are reading and writing it under the same lock, and the JLS already guarantees visibility.

[wind57]

this is overly complicated also, I think you are confusing what volatile is supposed to do and this confusion is "dragged" into this method: that is why you do a volatile read via the local httpClient. But again, this is not correct, you do not even need it, and the method can be simplified to :

@Override
public void close() {
	synchronized (this.lock) {
		if (this.sharedHttpClient == null) {
			return;
		}

		try {
			this.sharedHttpClient.close();
		}
		catch (IOException ex) {
			// Best-effort close during shutdown.
		}
		finally {
			this.sharedHttpClient = null;
		}
	}
}

[wind57]

same goes for this method, it can be simplified:

@Override
public ClientHttpRequestFactory get(
		SSLContext sslContext,
		@Nullable HostnameVerifier hostnameVerifier) {

	synchronized (this.lock) {
		if (this.sharedHttpClient == null) {
			HttpClientBuilder httpClientBuilder = HttpClientBuilder.create();

			if (sslContext != null
					|| hostnameVerifier != null
					|| timeoutProperties != null) {
				httpClientBuilder.setConnectionManager(
						buildConnectionManager(
								sslContext,
								hostnameVerifier,
								timeoutProperties));
			}

			this.sharedHttpClient = httpClientBuilder.build();
		}

		return new HttpComponentsClientHttpRequestFactory(
				this.sharedHttpClient);
	}
}

[wind57]

and now, if you look at the other comments, you will realize that this is not even needed anymore, you can simply make the methods synchronized...

[PRAHLAD09-dev]

Good point. I simplified the synchronization by making get() and close() synchronized methods and removed the explicit lock.