New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
close handlers and free all associated resources. #5535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me but let's have @dmlloyd check it.
/** | ||
* We can safely close log handlers after stop time has been printed. | ||
*/ | ||
InitialConfigurator.DELAYED_HANDLER.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would instead do something like:
for (Handler h : InitialConfigurator.DELAYED_HANDLER.clearHandlers()) {
try {
h.close();
} catch (Throwable ignored) {
}
}
This way a subsequent dev mode iteration is more likely to work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for the feedback David. Let me do the change.
Just waiting for CI now. |
I tested in my Windows 10 VM and I still see the error. It seems to be a bug in JBoss Logging ? (I managed to reproduce it by reducing the
|
I guess we must be missing this recent commit: jboss-logging/jboss-logmanager@f2e0eb3 |
I'll cherry-pick it and prep a release to go along with this change. |
Stupid question, but isn't it easier to bump Quarkus to JBoss LogManager 2.1.14.Final? |
No, this bug is in the JBoss LogManager, not in the logging API. We have a fork of the JBoss LogManager with changes to support embedded bootstrapping that has to be brought up to date. Before you ask, yes, I am working on upstreaming the changes so that we can go back to the upstream LogManager code at some point. |
Awesome, I corrected my comment above after I realized it's JBoss Logmanager. Thanks! |
I've released |
@machi1990 let me know when the PR is updated so I can test the changes in my Windows VM 😉 |
@machi1990 just FYI, better refresh this page than rely on the various Maven Central UIs (they are lagging a bit): https://repo1.maven.org/maven2/org/jboss/logmanager/jboss-logmanager-embedded/ |
Thanks for the heads up @gsmet |
@machi1990 You can also use https://www.artifact-listener.org/ to receive library updates in your e-mail or (as I do) use bash to do that for you 😉 https://gist.github.com/gastaldi/41bd76a2b539711a81943e3ff8254b1b |
Okay, this is super handy. Bookmarked. Thanks @gastaldi :-) |
I noticed that the handlers were never closed while trying to figure out the causes for quarkusio#5522. Fixes quarkusio#5522
Artifact Listener is updated at ~2:30pm Paris time every day so you have a delay too. I would know it, I created the project :). |
I have updated the PR with the log manager update. CI should fail, until we have |
https://repo1.maven.org/maven2/org/jboss/logmanager/jboss-logmanager-embedded/ is up to date and version 1.0.4 is available. Asked for a CI re-run. Let's wait and see. FYI @gsmet |
@gastaldi you wanted to test this. Also @mcrose confirmed that this fixed the issue for him #5522 (comment) |
Thanks @gsmet |
I noticed that the handlers were never closed while trying to figure out the causes for
#5522.
Fixes #5522