quarkus scheduler does not await termination of scheduledExecutor #16833
Comments
|
/cc @mkouba |
So first of all I don't think this is an acceptable solution - a scheduled task may take a long time to complete and the whole point of the hot replacement is to redeploy the app immediately. You're right that we don't wait for actively executing tasks. In fact, the scheduled tasks are executed on the default blocking Quarkus My understanding of the default settings for the blocking executor, i.e. |
|
I believe you are correct. It might be interesting/useful to set it to interrupt immediately in dev mode though? |
I agree. and at the same time it is very confusing.
strange. with that code: and interrupting just after starting run 3, I see: I don't see
yes that would be a pragmatic approach. now it does not solve the graceful issue. and may be it should be treated in a different PR. but basically you would want the to await termination up to |
Indeed, the thread is not interrupted at all. I'll need to debug the
I've created #16875 |
|
Ah, that shutdown task is not used in the dev mode at all. It seems that in the dev mode we do not shutdown the main executor for blocking tasks. @stuartwdouglas do you happen to know if there's still a need for this: https://github.com/quarkusio/quarkus/blob/main/core/runtime/src/main/java/io/quarkus/runtime/ExecutorRecorder.java#L29-L33 ? |
|
Probably not, lets see what CI says: #16920 |
|
So with Stuart's PR and my fix the current behavior should be as follows:
@vsevel Does it sound like an acceptable solution? |
so I suppose the compromise here is that we do not do an interrupt immediately on all managed threads?
here, the compromise is that we do not honor |
Yes, and it shouldn't be.
I'm not sure the semantics is exactly the same but in general yes, after the timeout quarkus does
+1. Would you care to contribute something?
I'm not sure TBH. |
sure.
it is either one of these 3 options:
my preference would be to go for option 2. if we go for option 1, then this will complicate the documentation and the user experience: if you want graceful shutdown and you have a scheduler and http endpoints, then you should probably align it looks like you are leaking an implementation detail. let me know what you decide, and I will make a proposal for the doc. |
|
I prefer option 2, i.e. Also note that we should not only talk about scheduler - the default thread pool is used for many other services (e.g. async CDI events). @machi1990 @stuartwdouglas WDYT? |
+1 on options (2) which sounds reasonable to me and easy to document. |
yes definitely. hopefully all use cases are known, and can adhere to |
|
@vsevel Is this still a problem? I do not see any PR as a result of the discussion so I wonder if this issue is either outdated or just forgotten ;-). |
|
I'm going to close this issue as it seems to be out of date. Feel free to reopen if it's still a problem. |
|
hello @mkouba sorry for the delay to answer. for the graceful shutdown use case, I do not think this is the expected behavior as well. When I would expect the ongoing activity to finish gracefully, but not restart new ones, as opposed to immediately interrupt threads. |
|
I took a look again and it's quite convoluted. Dev modeBlocking scheduled methods are usually executed on a Vert.x working thread (unless I don't think there's an easy way to stop/interrupt an action submitted with Prod modeAnd in the prod mode it's again connected to the fact that when the In theory, we could wrap the NOTE: I know that it would be a very ugly workaround but I see no better solution ATM 🤷 |
- in the prod mode we wrap the ExecutorService and the shutdown() and shutdownNow() are deliberately not delegated - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is closed before io.quarkus.runtime.ExecutorRecorder.createShutdownTask() is used - and when it's closed the underlying worker thread pool (which is in the prod mode backed by the ExecutorBuildItem) is closed as well - as a result the quarkus.thread-pool.shutdown-interrupt config property and logic defined in ExecutorRecorder.createShutdownTask() is completely ignored
Here's the draft PR: #38478. It seems to work. But of course it's not ideal. |
I think that we can do a similar trick for the dev mode - this time pass an "exchangeable" |
|
functionally, the most important part to me is a good support for graceful shutdown in prod mode. |
No, you cannot clear the execution queue. |
- in the prod mode we wrap the ExecutorService and the shutdown() and shutdownNow() are deliberately not delegated - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is closed before io.quarkus.runtime.ExecutorRecorder.createShutdownTask() is used - and when it's closed the underlying worker thread pool (which is in the prod mode backed by the ExecutorBuildItem) is closed as well - as a result the quarkus.thread-pool.shutdown-interrupt config property and logic defined in ExecutorRecorder.createShutdownTask() is completely ignored
- in dev mode we use a special executor for the worker pool where the underlying executor can be shut down and then replaced with a new re-initialized executor - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is reused between restarts but we must attempt to shut down this executor, for example to cancel/interrupt the scheduled methods
Actually, it's not only a visual thing. Those tasks may block the threads from the worker pool and make the app unresponsive... |
- in the prod mode we wrap the ExecutorService and the shutdown() and shutdownNow() are deliberately not delegated - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is closed before io.quarkus.runtime.ExecutorRecorder.createShutdownTask() is used - and when it's closed the underlying worker thread pool (which is in the prod mode backed by the ExecutorBuildItem) is closed as well - as a result the quarkus.thread-pool.shutdown-interrupt config property and logic defined in ExecutorRecorder.createShutdownTask() is completely ignored
- in dev mode we use a special executor for the worker pool where the underlying executor can be shut down and then replaced with a new re-initialized executor - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is reused between restarts but we must attempt to shut down this executor, for example to cancel/interrupt the scheduled methods
- in the prod mode we wrap the ExecutorService and the shutdown() and shutdownNow() are deliberately not delegated - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is closed before io.quarkus.runtime.ExecutorRecorder.createShutdownTask() is used - and when it's closed the underlying worker thread pool (which is in the prod mode backed by the ExecutorBuildItem) is closed as well - as a result the quarkus.thread-pool.shutdown-interrupt config property and logic defined in ExecutorRecorder.createShutdownTask() is completely ignored
- in dev mode we use a special executor for the worker pool where the underlying executor can be shut down and then replaced with a new re-initialized executor - this is a workaround to solve the problem described in quarkusio#16833 (comment) - the Vertx instance is reused between restarts but we must attempt to shut down this executor, for example to cancel/interrupt the scheduled methods
|
@mkouba I ran my reproducer with your branch. For the quarkus dev use case, I get this output upon restart so once the app restarts, I do not see anymore ticks from the old timer. But I see a interrupted exception, after startup. This means there was no graceful shutdown. In theory stopping the previous app should have waited for the ongoing tasks, stop then restart. is this what you expected? I ran the second use case: graceful shutdown, running the behavior looks good. I was thinking there would be a stopped log, like in this is that expected? |
@vsevel Yes, that's expected. I don't think that graceful shutdown makes any sense in the dev mode.
You're missing the |
fair enough
right
just in case it is a smell. thanks for looking into this issue. |
@vsevel Actually, the message is there! It's the INFO |
|
of course! one thing that tricked me was the thread difference. |
…on/other extensions Related to quarkusio#16833 quarkusio#43228
…on/other extensions Related to quarkusio#16833 quarkusio#43228
…on/other extensions Related to quarkusio#16833 quarkusio#43228
…on/other extensions Related to quarkusio#16833 quarkusio#43228
…on/other extensions Related to quarkusio#16833 quarkusio#43228
Describe the bug
I execute the following code inspired from https://quarkus.io/guides/scheduler:
When hot code replace happens in dev mode, the scheduler extension does not wait for the
scheduledExecutorto terminate.as a result, the old timer continues executing the tasks that are in progress in parallel with the new tasks scheduler by the new timer.
Eventually, the old code finishes up, and only the new timer remains.
For instance, here is how it looks like with the previous example:
Note that this behavior is less visible, but happens also in production mode: in Graceful shutdown, we should also wait for the termination of tasks that are in progress.
I think here is the relevant code.
Expected behavior
Stopping the timer, awaiting termination, then restart the application.
Actual behavior
Old timers keep running until they finish up normally.
If we do not await termination, another option would be to interrupt the threads.
To Reproduce
Link to a small reproducer (preferably a Maven project if the issue is not Gradle-specific).
Execute the above code in a scheduler application
Configuration
# Add your application.properties here, if applicable.Screenshots
(If applicable, add screenshots to help explain your problem.)
Environment (please complete the following information):
Output of
uname -aorverOutput of
java -versionGraalVM version (if different from Java)
Quarkus version or git rev
Quarkus 1.13.2.Final
Build tool (ie. output of
mvnw --versionorgradlew --version)Additional context
(Add any other context about the problem here.)
The text was updated successfully, but these errors were encountered: