Shutdown hook in VertxHttpClientFactory prevents further client usage

[tdiesler]

Describe the bug

This library should not assume that it is in charge of shutting down the Http connection. Instead, this should be left to the client app.
In camel k8s we have we have a 'run' option which can be stopped with ctrl+c, upon which all k8s resources are also deleted from the cluster.

This code causes regression, such that these resources can no longer be deleted when executing our shutdown hook ...

    public VertxHttpClientFactory() {
        this(VertxHttpClientFactory.VertxHolder.INSTANCE);
        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
            if (this.vertx != null) {
                this.vertx.close();
            }

        }));
    }

I checked the other http client variants and it seems that vertx is the only one that installs a shutdown hook. Please consider removing this code and instead rely on higher layers to close the k8s client when appropriate.

Fabric8 Kubernetes Client version

7.0.1

Steps to reproduce

See: https://issues.apache.org/jira/browse/CAMEL-21621

Expected behavior

Using this k8s client in a shutdown hook should be valid

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.25.3@latest

Environment

Linux

Fabric8 Kubernetes Client Logs

Additional context

No response

[manusa]

Hi @tdiesler

I understand that this issue is in scope of https://issues.apache.org/jira/browse/CAMEL-21625

The VertxHttpClientFactory provides two constructors:

A no args constructor:

kubernetes-client/httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientFactory.java

Line 58 in 1abb411

public VertxHttpClientFactory() {

And one that accepts your own Vertx instance as a parameter:

kubernetes-client/httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientFactory.java

Line 67 in 1abb411

public VertxHttpClientFactory(Vertx vertx) {

The shutdown hook is only added to the prior one.
This is mainly because it's assumed that the Kubernetes Client owns it and is responsible for its lifecycle.

AFAIU, it's expected that the Vertx instance is closed before the application is terminated.
I also assume that this is especially important in contexts such as the Kubernetes Client, where you might have dangling threads running when SIGTERM is received.

The only reliable and simple way I found out to ensure this behavior is by leveraging the shutdown hook. Since the factory can be instantiated multiple times and the Vertx instance is a singleton shared among all the factories (in case you use the default empty args constructor), having a callback to close the Vertx instance would be rather complicated. In addition, you would probably face the same issue.

The only problem is that this method is not compatible with having other shutdown hooks that use the client and the default constructor factory, since depending on which hook run firsts you might run into the problem you describe.

So unless there's another way or it's OK not to close the Vert.x instance, the only way is for you to use the factory with a Vertx instance that you manage yourself.

/cc @shawkins @vietj

[tdiesler]

I'd say, that this library cannot "own" the client instance and its dependent vertx instance. Instead, whoever creates the k8s client is also responsible of closing it appropriately, which may happen in a shutdown hook. This would also align the vertx client with how the other http client variants work.

In other words, this library cannot possibly know when the application is done with using the k8s client. The assumption that this is true when it's own shutdown hook is called, is not valid.

[manusa]

Instead, whoever creates the k8s client is also responsible of closing it appropriately, which may happen in a shutdown hook.

That's fine, but we're talking about the client factory here (which by extension also implies the constructed client instances, sure).

The problem is that the factory should close the resources it creates, in this case the Vertx instance.
And there's no feasible way to achieve this.

Your current proposal is to leave this Vertx instance open which might or might not have serious consequences downstream. But I'm not sure what the consequences of not closing the Vertx instance are.

I'm perfectly fine removing the hook, but I'd like to see the opinion on this matter from someone more knowledgeable on Vertx.

[tdiesler]

We use code like this ...

        var client = new KubernetesClientBuilder().withConfig(config).build();
        printClientInfo(client);

we are (or it least should be) unaware of the http implementation details of the returned client. We assume responsibility to close the client when done ... and would prefer the k8s library not to make that decision for us. Yes, please remove that hook if you can.

[manusa]

I know and I understand, but we're talking about the factory, not the client.

Removing the shutdown hook might have unexpected consequences.

The current approach is only problematic in case of users (like you) using the client in a shutdown hook.
It's easier/simpler to add a warning and encourage users to use their own factory (which is a very simple change) in this case, than to force the entire user-base to close a client factory they might not even be aware of.

As I said, if someone knowledgeable in Vert.x assures us that there is no problem in not closing the Vertx instance, then I'm all in favor of removing the shutdown hook.
Otherwise, we need to properly analyze the tradeoffs of the available approaches.

[tdiesler]

I'd say it is a common convention in Java that when you acquire a closable resource - you also close it.

Like so ....

        devModeShutdownTask = new Thread(() -> {
            try (var client = createKubernetesClient()) {
                ... do stuff
            } catch (Exception e) {
                throw new RuntimeException(e);
            }
        });
       Runtime.getRuntime().addShutdownHook(devModeShutdownTask);

The code is both simple and correct. However, Java does not allow to register additional shutdown hooks while already in shutdown. Therefore, it'd be wrong for a library to assume that it can create a shutdown hook.

In case a closable resource A is shared by multiple other closable resources B, you would need a usage counter in B i.e. the last B.close() would call A.close()

If you really must have that shutdown hook - how about, you ignore the exception thrown by Runtime.getRuntime().addShutdownHook() in the factory and document the intricate details of why you ignore it. Not a clean solution though, because it might ignore exceptions that should in fact be thrown to the caller.

[manusa]

I'd say it is a common convention in Java that when you acquire a closable resource - you also close it.

Yes. I insist, we're talking about the factory, which IS NOT closable and not exposed to the user.
The shutdown hook IS NOT closing the client or clients, but the underlying vertx instance (which obviously renders the underlying clients unusable).

If you really must have that shutdown hook - how about, you ignore the exception thrown by Runtime.getRuntime().addShutdownHook() in the factory and document the intricate details of why you ignore it. Not a clean solution though, because it might ignore exceptions that should in fact be thrown to the caller.

Yes, the idea of this tradeoff, is to make it as safe as possible.
And also document the warning and alternative path in case you need to use the client in a shutdown hook (your exposed use-case).

Either this or finding a better solution, but I'm out of ideas.

[tdiesler]

Ok, here again for future reference ... this code is broken in the k8s client ...

new KubernetesClientBuilder().withConfig(config).build();

[manusa]

Another alternative is that the default factory creates a Vertx instance per KubernetesClient instance.
However, I'm not sure how this would affect the overall performance.

[shawkins]

@manusa was the impetus to add a shutdown hook just in case vertx was still using non-daemon threads? cc @vietj

@tdiesler if I understand the issue correctly this really comes down to an ordering of the shutdown hooks - since they are run concurrently there's no way to control the ordering and the vertx shutdown is happening before your other hooks get a chance to complete.

Assuming that the best default behavior is to install the shutdown hook, to give users more control we can do one or more of the following:

• use a system property to control the installation of the shutdown hook
  • if we then also want to give users the ability to do their own final closure, then either the static vertx instance will need exposed, or we should refactor the logic so that users can call createVertxInstance to use the constructor that takes an instance. Either way this will require a compile time vertx client dependency.
• move the addition of the shutdown hook to the createVertxInstance and expose the shutdown hook Thread as a static variable that can be accessed via the factory. Users affected by this scenario could then use Runtime.getRuntime().removeShutdownHook(VertxHttpClientFactory.getShutdownHook()) and take ownership over the shutdown. This again requires a compile time dependency on the vertx client.
• A long shot - add an api for the shutdown ordering. Effectively expose a CountDownLatch somehow (if it's just for vertx, then yet again this will require a compile dependency) that the user can increment for each shutdown hook they add, and then must decrement when the hook is done running.

[manusa]

@manusa was the impetus to add a shutdown hook just in case vertx was still using non-daemon threads? cc @vietj

Yup.

What I'm unsure about is, given that the factory is setting Vert.x to use daemon threads, what if we just omit closing the Vertx instance.
If there's no issue with that, then we can just go ahead and remove the shutdownhook altogether.

Considering this is a bad practice then:

Assuming that the best default behavior is to install the shutdown hook, to give users more control we can do one or more of the following:

This one A long shot - add an api for the shutdown ordering., I wouldn't do, too much complexity and JDK internals. I understand that the JDK doesn't guarantee the execution order of shutdown hooks for a reason, this might tamper with that reasoning.

The other ones are good ideas that we can polish.

On the other hand, there was still the option to create a Vertx instance per VertxHttpClient instance which can then be closed alongside the client. Not sure about the effects on performance of this one.

[shawkins]

If there's no issue with that, then we can just go ahead and remove the shutdownhook altogether.

Right, hopefully @vietj can confirm.

On the other hand, there was still the option to create a Vertx instance per VertxHttpClient instance which can then be closed alongside the client. Not sure about the effects on performance of this one.

I had started to look into that with #6709 - I had mistakenly said that it's reusing the same threadpool. It's not, only same default factories for creating the ThreadFactories and ExecutorServices. Creating 1 vertx per Kuberentes HttpClient shouldn't be categorically different than the resource usage / setup cost than is seen with the OkHttp or Jetty implementations, so I'm fine with that too.

[manusa]

Creating 1 vertx per Kuberentes HttpClient shouldn't be categorically different than the resource usage / setup cost than is seen with the OkHttp or Jetty implementations, so I'm fine with that too.

This seems the cleaner approach to me then.
I'll prototype it on Monday, and see how it goes.

[manusa]

@tdiesler posted in #6800 (comment)

This API is "broken" in a sense that it returns an object that cannot be used (anymore) in all intended scenarios. The workaround that I came up with is here.

[manusa]

Creating 1 vertx per Kuberentes HttpClient shouldn't be categorically different than the resource usage / setup cost than is seen with the OkHttp or Jetty implementations, so I'm fine with that too.

This seems the cleaner approach to me then. I'll prototype it on Monday, and see how it goes.

Initial prototype using a Vert.x instance per VertxHttpClientBuilder->VertxHttpClient instance.
The exclusive instance is closed whenever a VertxHttpClient constructed from the VertxHttpClientBuilder is closed.

The factory still provides the option to use a shared Vertx instance across all VertxHttpClient instances.