[Fabric8] `KubernetesClient` is closed too early during shutdown

[kzander91]

Any KubernetesClient beans are closed right at the start of the shutdown procedure, when the ContextClosedEvent is published:

spring-cloud-kubernetes/spring-cloud-kubernetes-fabric8-autoconfig/src/main/java/org/springframework/cloud/kubernetes/fabric8/Fabric8AutoConfiguration.java

Lines 124 to 131 in 30322ea

	@EventListener
	void onContextClosed(ContextClosedEvent event) {
	// Clean up any open connections from the KubernetesClient when the context is
	// closed
	BeanFactoryUtils.beansOfTypeIncludingAncestors(event.getApplicationContext(), KubernetesClient.class)
	.values()
	.forEach(Client::close);
	}

However, this is too early, causing subsequent failures during the stop() procedure of LeaderInitiator, which attempts to revoke its leadership here:

spring-cloud-kubernetes/spring-cloud-kubernetes-commons/src/main/java/org/springframework/cloud/kubernetes/commons/leader/LeaderInitiator.java

Line 80 in 30322ea

leadershipController.revoke();

Revoking leadership requires an API call, but that is now failing because the client has already been closed at this point:

java.lang.IllegalStateException: Client is closed
	at io.vertx.core.http.impl.HttpClientImpl.checkClosed(HttpClientImpl.java:405)
	at io.vertx.core.http.impl.HttpClientImpl.doRequest(HttpClientImpl.java:281)
	at io.vertx.core.http.impl.HttpClientImpl.request(HttpClientImpl.java:191)
	at io.fabric8.kubernetes.client.vertx.VertxHttpRequest.consumeBytes(VertxHttpRequest.java:87)
	at io.fabric8.kubernetes.client.vertx.VertxHttpClient.consumeBytesDirect(VertxHttpClient.java:131)
	at io.fabric8.kubernetes.client.http.StandardHttpClient.consumeBytesOnce(StandardHttpClient.java:109)
	at io.fabric8.kubernetes.client.http.StandardHttpClient.lambda$consumeBytes$1(StandardHttpClient.java:90)
	at io.fabric8.kubernetes.client.utils.AsyncUtils.retryWithExponentialBackoff(AsyncUtils.java:75)
	at io.fabric8.kubernetes.client.utils.AsyncUtils.retryWithExponentialBackoff(AsyncUtils.java:68)
	at io.fabric8.kubernetes.client.http.StandardHttpClient.retryWithExponentialBackoff(StandardHttpClient.java:163)
	at io.fabric8.kubernetes.client.http.StandardHttpClient.consumeBytes(StandardHttpClient.java:88)
	at io.fabric8.kubernetes.client.http.SendAsyncUtils.bytes(SendAsyncUtils.java:50)
	at io.fabric8.kubernetes.client.http.HttpResponse$SupportedResponses.sendAsync(HttpResponse.java:104)
	at io.fabric8.kubernetes.client.http.StandardHttpClient.sendAsync(StandardHttpClient.java:75)
	at io.fabric8.kubernetes.client.dsl.internal.OperationSupport.handleResponse(OperationSupport.java:547)
	at io.fabric8.kubernetes.client.dsl.internal.OperationSupport.handleResponse(OperationSupport.java:524)
	at io.fabric8.kubernetes.client.dsl.internal.OperationSupport.handleGet(OperationSupport.java:467)
	at io.fabric8.kubernetes.client.dsl.internal.BaseOperation.handleGet(BaseOperation.java:792)
	at io.fabric8.kubernetes.client.dsl.internal.BaseOperation.requireFromServer(BaseOperation.java:193)
	at io.fabric8.kubernetes.client.dsl.internal.BaseOperation.get(BaseOperation.java:149)
	at io.fabric8.kubernetes.client.dsl.internal.BaseOperation.get(BaseOperation.java:98)
	at org.springframework.cloud.kubernetes.fabric8.leader.Fabric8LeadershipController.getConfigMap(Fabric8LeadershipController.java:155)
	at org.springframework.cloud.kubernetes.fabric8.leader.Fabric8LeadershipController.lambda$revoke$2(Fabric8LeadershipController.java:85)
	at org.springframework.cloud.kubernetes.commons.leader.LeaderUtils.guarded(LeaderUtils.java:51)
	at org.springframework.cloud.kubernetes.fabric8.leader.Fabric8LeadershipController.revoke(Fabric8LeadershipController.java:84)
	at org.springframework.cloud.kubernetes.commons.leader.LeaderInitiator.stop(LeaderInitiator.java:80)
	at org.springframework.cloud.kubernetes.commons.leader.LeaderInitiator.stop(LeaderInitiator.java:87)
	at org.springframework.context.support.DefaultLifecycleProcessor.doStop(DefaultLifecycleProcessor.java:463)
	at org.springframework.context.support.DefaultLifecycleProcessor$LifecycleGroup.stop(DefaultLifecycleProcessor.java:618)
	at java.base/java.lang.Iterable.forEach(Unknown Source)
	at org.springframework.context.support.DefaultLifecycleProcessor.stopBeans(DefaultLifecycleProcessor.java:432)
	at org.springframework.context.support.DefaultLifecycleProcessor.onClose(DefaultLifecycleProcessor.java:323)
	at org.springframework.context.support.AbstractApplicationContext.doClose(AbstractApplicationContext.java:1172)
	at org.springframework.boot.web.reactive.context.ReactiveWebServerApplicationContext.doClose(ReactiveWebServerApplicationContext.java:155)
	at org.springframework.context.support.AbstractApplicationContext.close(AbstractApplicationContext.java:1126)
	at org.springframework.boot.SpringApplicationShutdownHook.closeAndWait(SpringApplicationShutdownHook.java:147)
	at java.base/java.lang.Iterable.forEach(Unknown Source)
	at org.springframework.boot.SpringApplicationShutdownHook.run(SpringApplicationShutdownHook.java:116)
	at java.base/java.lang.Thread.run(Unknown Source)

Additionally, since LeaderInitiator.stop() failed before it could set isRunning = false, and its bean definition unnecessarily declares the stop() method as a custom destroy method here,

spring-cloud-kubernetes/spring-cloud-kubernetes-fabric8-leader/src/main/java/org/springframework/cloud/kubernetes/fabric8/leader/Fabric8LeaderAutoConfiguration.java

Line 104 in 30322ea

@Bean(destroyMethod = "stop")

an additional exception is thrown right after:

Custom destroy method 'stop' on bean with name 'leaderInitiator' propagated an exception: java.lang.NullPointerException: Cannot invoke "java.util.concurrent.ScheduledExecutorService.shutdown()" because "this.scheduledExecutorService" is null

This happens because the first invocation of stop() has unset the scheduledExecutorService, but failed before setting the isRunning flag to false, causing the second invocation to go through here again, now failing with an NPE.

To fix this, I propose to remove (destroyMethod = "stop") from LeaderInitiator's bean definition (stop() is a Lifecycle method and thus invoked automatically by the container at the correct time), and to remove the onContextClosed event listener in Fabric8AutoConfiguration (KubernetesClient#close is inherited from AutoCloseable and thus also invoked automatically by the container at the correct time, namely, after Lifecycle.stop()).

[wind57]

To fix this, I propose to remove (destroyMethod = "stop") from LeaderInitiator's bean definition (stop() is a Lifecycle method and thus invoked automatically by the container at the correct time)

This is already fixed: #1953

Will look at the second part too. Thank you

[wind57]

I don't know if we should implement the second part that you mention here. I see we have this PR that introduced that : #1780 so not sure..

[kzander91]

The discussion in #1780 didn't really go into detail about what the exact issue was that it was trying to solve. But if I use some debugger shenanigans to skip the ContextClosedEvent, everything seems to work fine on my machine and the KubernetesClient is closed after the LeaderInitiator has completed its stop() procedure.
Perhaps it would be worth re-investigating the issue that #1780 was actually trying to solve. Maybe there were issues in Spring Framework itself that have since been fixed/improved?

In any case, the status quo is unacceptable, as the KubernetesClient is definitely closed too early, causing the subsequent failures every time, and potentially other errors if in-progress operations are trying to use it during a graceful shutdown.

[wind57]

you are correct, in principle. We are trying to figure out why that code exists and what it was trying to fix when it was created.