Quarkus - SmallryeHealth - PreDestroy not called on HealthCheck #38878
Comments
|
/cc @Ladicek (smallrye), @geoand (kotlin), @jmartisk (health,smallrye), @phillip-kruger (smallrye), @radcortez (smallrye), @xstefank (health) |
|
I'll clone the reproducer and will investigate properly, but at the first sight, the Also, the fact that a request-scoped bean is injected anywhere or what's the scope of the bean that injects it should not matter. A request-scoped bean must be destroyed when the request context terminates. |
|
@Ladicek for your awareness, if you are having a look to this one. There are a few other oddities in progress:
Might not be related but better have the big picture :) |
|
Thanks @gsmet, I'll keep that in mind! |
|
Ah the README in the reproducer has an interesting bit of information: the issue occurs when calling the health check, but when calling any other endpoint, it doesn't. So the links above are indeed very much relevant. |
Hi @Ladicek , |
|
This is caused by manual Uni caching done in SmallRye Health for performance reasons. To correctly handle CDI context propagation, you need to set the |
|
Caching is fine if you handle the duplicated context correctly - See #37077 |
|
Hello @xstefank , I have tried adding the property
|
What exactly are you caching? |
|
@olivierbeltrandocintoo There was another issue that collided with this and will be fixed with #38888. With this PR, your reproducer works. @gsmet I will need to fix it. In SR Health, when you invoke health, we compute a Uni for every major health group, let's say for readiness, we have readinessUni that is saved. On the next invocation, we just return this saved readinessUni once called again so Quarkus can resubscribe and call all end Uni subscriptions (health checks) again. But we save the finding and collecting of health checks into a parent Uni. |
|
Hi ! I run the reproducer against Quarkus 3.7.4 that contains the fix #38888 and you are totally right, the |
|
Just for the record, I will investigate whether we can also propagate context without recreating Unis. |
|
@xstefank When we talked about this the other day, I learned that what we do is actually more correct than I thought, but it also got me thinking. On each call to a health endpoint, how many Say that calling a health endpoint requires calling N health checks. We have cached a |
|
This same issue exists with Kotlin code, and is not solved. Here is the reproducer (same code as in Java but in Kotlin): https://github.com/olivierbeltrandocintoo/quarkus-health-noPreDestroy-kotlin Also it has been like this for as long as I know :) Also would you like me to open another ticket for this issue ? |
|
If a |
|
Hi @Ladicek , this part behaves nicely (I wrongly set the counter as static in my example). Thanks :) I will monitor then how it behaves once the Kotlin version calls the preDestroy. What do you think about the issue in Kotlin ? |
|
I can't think of a reason why it should be different between Java and Kotlin. |
@olivierbeltrandocintoo Is this still a problem? Have you observed an incorrect behavior in Kotlin? |
|
Thanks for the ping. I have deactivated those in the meantime because the issue was not fixed. Let me check again and get back to you :) |
|
Hi, so unfortunately the issue is still persisting even with latest versions of Quarkus (3.14.4) and Kotlin (from the bom 2.0.10). Don't hesitate to use this project as reproducer: https://github.com/olivierbeltrandocintoo/quarkus-health-noPreDestroy-kotlin
Thanks :) |
|
@olivierbeltrandocintoo Thanks! Will take a look... |
|
I can confirm that the @PostConstruct
void init() {
Log.warn("Init called");
}
@PreDestroy
void destroy() {
Log.warn("Destroy called");
} change the scope to I'm actually quite surprised that it's not tested by the MP TCK. The spec does not explicitly require the support for |
|
@mkouba it's not, app scoped health checks are normal. I would think that handling of PreDestroy is CDI responsibility, not something I should do manually in sr-health. Except for Dependent health checks which are fixed (not yet in Quarkus) smallrye/smallrye-health#563. |
I'm sorry, you have to change the scope of |
|
Please keep in mind that originally this ticket described a bug that was present in both java and kotlin. And the bug was fixed for Java (see this repo https://github.com/olivierbeltrandocintoo/quarkus-health-noPreDestroy). The Kotlin code tries to be similar to the java but the endOfRequest is not called. |
I don't think it was fixed for Java... |
|
For the record, I would be strongly in favor of removing the |
|
+100 |
We could do it but this solution just masks the original problem. I was digging in the code and did some experiments and it seems that the non-cached solution (with Now, if I replace I believe that health checks should be executed on the same Vertx context that is used to proceed the HTTP request. But I'm no SR Health/Vertx expert. Also the necessity of using SR CP just for a health check seems like an overkill to me. |
+100 |
|
We can keep using the current duplicated context if we have one and create a new only if we are called from a root context (or no context). In the latter case, activating the request scope can be an option (in the execute blocking block) |
|
We had to introduce new vertx context for MDC logs propagation which are coupled to it. I'll give a shot to what @cescoffier is proposing if that would still work. |
Describe the bug
I expect that having a class implementing
HealthCheckthat gets injected aRequestScopedobject will call the@PreDestroymethod of the injected object at the end of the check.Expected behavior
On each call to http://localhost:8080/q/health/ the PreDestroy is called on the HealtCheck marked as RequestScoped
Actual behavior
On Quarkus 3.6.7:
On call http://localhost:8080/q/health/ the PreDestroy is called only the first time.
On Quarkus 3.7.3
On call http://localhost:8080/q/health/ the PreDestroy is never called.
How to Reproduce?
Clone this repo based on the SmallRyeHealth tutorial
https://github.com/olivierbeltrandocintoo/quarkus-health-noPreDestroy
Note: I usually work in
Kotlinbut to have the simplest minimal case, I made it inJava:)Call the health endpoints and see that in the logs that the PreDestroy is either not called (for quarkus 3.7.3) or only called the first time (for quarkus 3.6.7)
logs related
Output of
uname -aorver5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Output of
java -versionOpenJDK Runtime Environment (build 17+35-2724)
Quarkus version or git rev
3.7.3 and 3.6.7
Build tool (ie. output of
mvnw --versionorgradlew --version)Apache Maven 3.9.6
Additional information
No response
The text was updated successfully, but these errors were encountered: