Skip to content

[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

Merged
merged 4 commits into from
Feb 18, 2022

Conversation

ppkarwasz
Copy link
Contributor

When a default PatternLayout is created without a Configuration a new DefaultConfiguration is created and abandoned. This causes a leak of ConsoleManagers.

This PR provides a Configuration argument whenever possible.

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.
Copy link
Contributor

@carterkozak carterkozak left a 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.

…r/AbstractManager.java

Co-authored-by: Carter Kozak <ckozak@ckozak.net>
@ppkarwasz
Copy link
Contributor Author

@carterkozak: I'll check if we can leave the Configuration as null and update the PR.

if (configuration == null) {
configuration = new DefaultConfiguration();
}
// should work with a null configuration
Copy link
Contributor Author

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 PatternConverters.

Looking at the available PatternConverters, 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.

@ppkarwasz
Copy link
Contributor Author

@carterkozak: that should do it.

As far as I can see, no NPEs will result from providing null as configuration in the PatternLayout. I added a null check for the only case I found.

Copy link
Contributor

@carterkozak carterkozak left a 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!

@carterkozak carterkozak merged commit 9ffa643 into apache:release-2.x Feb 18, 2022
@ppkarwasz ppkarwasz deleted the config-leak branch March 25, 2022 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants