-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[LOG4J2-3404] Creates default layouts using the available Configuration #756
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
Conversation
When a default `PatternLayout` is created without a `Configuration` a new `DefaultConfiguration` is created and abandoned. This causes a leak of `ConsoleManager`s. This PR provides a `Configuration` argument whenever possible.
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.
Change looks good!
I think that new DefaultConfiguration()
in PatternLayout.Builder.build()
is a bug, perhaps the layout itself should handle a null configuration gracefully? Not necessarily a requirement for this PR as we're ensuring we're passing the correct reference everywhere, but I think the new DefaultConfiguration()
that doesn't come from a ConfigurationFactory
has high likelihood of causing similar problems again in the future.
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractManager.java
Outdated
Show resolved
Hide resolved
…r/AbstractManager.java Co-authored-by: Carter Kozak <ckozak@ckozak.net>
@carterkozak: I'll check if we can leave the |
if (configuration == null) { | ||
configuration = new DefaultConfiguration(); | ||
} | ||
// should work with a null configuration |
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.
This requires more explanations: the PatternParser
does not use a configuration directly (or has null checks), but allows one to be injected into the PatternConverter
s.
Looking at the available PatternConverter
s, the color converters, "enc", "equals*", "highlight", "maxLength", "replace", "style", "ex*" require a configuration to create a PatternParser
.
I didn't find any converters except LiteralPatternConverter
that actually use Configuration
. LiteralPatternConverter
has appropriate null checks.
7befdbb
to
2d67b4c
Compare
@carterkozak: that should do it. As far as I can see, no NPEs will result from providing |
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.
Great work, thank you for the contribution!
When a default
PatternLayout
is created without aConfiguration
a newDefaultConfiguration
is created and abandoned. This causes a leak ofConsoleManager
s.This PR provides a
Configuration
argument whenever possible.