[LOG4J2-3634] move invocation of propagateLogLevels() #1140
Conversation
in order to wait for the handlers to be registered in the root logger
mknet
changed the title
There was a problem hiding this comment.
Thank You for your contribution. Except two small synchronization issues, it looks good to me.
While this is an issue specific to Tomcat, which fails to properly configure the root logger if Logger.getLogger("") is called by any handler constructor, I am afraid that many other LogManager implementations may suffer from this bug. Therefore you solution is the cleanest we can get.
log4j-jul/src/main/java/org/apache/logging/log4j/jul/Log4jBridgeHandler.java
Outdated
Show resolved
Hide resolved
log4j-jul/src/main/java/org/apache/logging/log4j/jul/Log4jBridgeHandler.java
Outdated
Show resolved
Hide resolved
Thanks for your reply and feedback, @ppkarwasz |
|
@ppkarwasz It seems to me, the code format wasn't correct. I re-ran spotless and committed the change. Sorry for the frustration. |
| @SuppressWarnings("resource") // no need to close the AutoCloseable ctx here | ||
| LoggerContext context = LoggerContext.getContext(false); | ||
| context.addPropertyChangeListener(this); | ||
| propagateLogLevels(context.getConfiguration()); | ||
| // note: java.util.logging.LogManager.addPropertyChangeListener() could also | ||
| // be set here, but a call of JUL.readConfiguration() will be done on purpose | ||
| this.installAsLevelPropagator = false; |
There was a problem hiding this comment.
By looking at some established practices for lazy initialization (e.g. Commons Lang), there should be a second if (this.installAsLevelPropagator) inside the synchronized block to ensure that propagateLogLevels is called only once.
In the master branch we do have a generic LazyUtil, but it is probably not worth backporting it for a single callsite.
There was a problem hiding this comment.
It depends if other lazy initialized code could be refactored in this branch, too. Can look for volatile fields for starters.
There was a problem hiding this comment.
installAsLevelPropagator must also be volatile
There was a problem hiding this comment.
Oops I thought volatile, but I wrote transient. Thanks, Carter.
There was a problem hiding this comment.
Uhhhh, now it makes sense to me as well. I will modify the code tomorrow
There was a problem hiding this comment.
@ppkarwasz @carterkozak volatile is applied. Anything else? Thx!
There was a problem hiding this comment.
@mknet: a second check for installAsLevelPropagator is needed so that if two thread enter in the first conditional block one will update the levels and the other will wait until it is done:
if (this.installAsLevelPropagator) {
synchronized(this) {
if (this.installAsLevelPropagator) {
There was a problem hiding this comment.
Check. Will come back to you in the next couple of hours.
There was a problem hiding this comment.
@ppkarwasz Back again and committed the change including some describing comments.
As for me, I would like to clean-up the code at other lines as well. But first things first.
|
Thank you for finding and fixing this bug. |
|
@ppkarwasz It was a pleasure! Thanks for your support. |
See https://issues.apache.org/jira/browse/LOG4J2-3634