eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.32k stars 2.08k forks source link

ShutdownHook #4230

Closed knotenpunkt closed 2 years ago

knotenpunkt commented 2 years ago

The vertx internal ShutdownHook calls vertx.close();

The problem here is, that it is not waiting on finish, because the internal returned Future is not blocked.

So i thought to make my own ShutdownHook, but then it is race between my ShutDownHook and the Vertx internal.

If i call vertx.close() after the internal hook called it already, then I get a new Future.

So maybe here would be two improvments good:

tsegismont commented 2 years ago

Which part of the code are you referring to?

To my knowledge, there's a hook registered by the CLI (or the launcher) and it does wait for the Vert.x to be properly closed:

https://github.com/eclipse-vertx/vert.x/blob/601baccefd6f108b561627ec240ed46ee81d2f93/src/main/java/io/vertx/core/impl/launcher/commands/BareCommand.java#L454-L476

knotenpunkt commented 2 years ago

good backquestion.

i thought iIfound the location in the code in the past, but I can't find it anymore, hmmm The behavior I described, is the same as here in the answer in stackoverflow: https://stackoverflow.com/a/69114433/2182302

The method stop() is invoked when Vert.x undeploys a verticle.

When terminating your application, Vert.x will attempt to undeploy the verticles as well, but it's a race between event loop still running and your application shutting down.

But one thing i can invalidate in my issue. The cached Future is not needed, because there is a check:

in VertxImpl.java

  @Override
  public synchronized void close(Handler<AsyncResult<Void>> completionHandler) {
    if (closed || eventBus == null) {
      // Just call the handler directly since pools shutdown
      if (completionHandler != null) {
        completionHandler.handle(Future.succeededFuture());
      }
      return;
    }
    closed = true;

    //...

But anyway, maybe its the correct behavior or an other bug:

my own ShutdownHook:

        Runtime.getRuntime().addShutdownHook(new Thread(() -> {

            this.vertx.close().toCompletionStage().toCompletableFuture().join();
            log.info("SHUTDOWN");
        }));

The join seems to wait infinite. The stop-methods in my verticles are called, I see there the debug-messages. But the log.info("SHUTDOWN";) is not executed and it hangs.

Could it also be possible, that my verticles aren't undeployed completely also if the stop method of the verticles are executed completely?

And the following

 Runtime.getRuntime().addShutdownHook(new Thread(() -> {

            this.vertx.close().onComplete(element ->{
                System.out.println(element);
                System.out.println("done");
            });

            this.vertx.close().toCompletionStage().toCompletableFuture().join();

            log.info("SHUTDOWN");
        }));

And thats interesting this code. Here the second vertx.close() gets the succeededFuture directly, before vertx is closed. So the join() will not block. Maybe we should return here really not a second future, but really the same future.

I did there a third variant. Using for the onComplete.... and the join() the same returned Future. They are both not called, so I guess the undeploy is not completed full. Where can I find, what prevents the full undeployment?

tsegismont commented 2 years ago

You should check that your verticles stop methods actually complete the promise passed as parameter.

I'm closing this one as it seems it is not a defective behavior. Feel free to reopen if you can provide a small reproducer which shows a Vert.x bug.

knotenpunkt commented 2 years ago

yeah you are right, i forgot to complete the promise^^ (good analytic-skills, you have)

But anyway, to call the vertx.close() a second time returns always a succeedFuture back, also if the thirst call is still in process. So i think here we can have a small improvement to return not new Futures back?

What do you think about that, @tsegismont ?

it is that code here:

 @Override
  public synchronized void close(Handler<AsyncResult<Void>> completionHandler) {
    if (closed || eventBus == null) {
      // Just call the handler directly since pools shutdown
      if (completionHandler != null) {
        completionHandler.handle(Future.succeededFuture());
      }
      return;
    }
    closed = true;

    //...
tsegismont commented 2 years ago

But anyway, to call the vertx.close() a second time returns always a succeedFuture back, also if the thirst call is still in process. So i think here we can have a small improvement to return not new Futures back?

Perhaps that could be improved by creating a closeFuture when the method is invoked for the first time and then notify the completionHandler passed in subsequent calls.

But I'm not convinced this is worth the effort.