Skip to content

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