Skip to content

avoid null classloader when thread context CL is null#464

Closed
jackshirazi wants to merge 1 commit into
apache:mainfrom
jackshirazi:juli-filehandler-cl-fix
Closed

avoid null classloader when thread context CL is null#464
jackshirazi wants to merge 1 commit into
apache:mainfrom
jackshirazi:juli-filehandler-cl-fix

Conversation

@jackshirazi

Copy link
Copy Markdown

I've seen the config get ignored when thread context classloader is null - if the cl is left at null, the default OneLineFormatter is used instead of the configured formatter

I've seen the config get ignored when thread context classloader is null - if the cl is left at null, the default OneLineFormatter is used instead of the configured formatter
@markt-asf

Copy link
Copy Markdown
Contributor

I'm curious as to what scenario triggers this issue.

It looks like this isn't the only place in JULI that needs to handle this case. I see at least one other place where null isn't handled.

Generally, the solution should be consistent with https://github.com/apache/tomcat/blob/main/java/org/apache/juli/ClassLoaderLogManager.java#L400

It may be that some refactoring would be helpful.

@jackshirazi

Copy link
Copy Markdown
Author

Triggered in my case when an agent (in this case Elastic APM agent) was also present, and the agent needs to do classloader manipulation for instrumentation.

I suspect getClass().getClassLoader() is a better choice here than ClassLoader.getSystemClassLoader(), but I'm ambivalent

I don't see where else in JULI the case is present? In the FileHandler.newThread() it's valid for the loader to be null

@markt-asf

Copy link
Copy Markdown
Contributor

For Tomcat getClass().getClassLoader() will be the same as ClassLoader.getSystemClassLoader() but I agree that generally, getClass().getClassLoader() is the better choice.
I'm working on a slightly wider patch that makes this more consistent throughout JULI.

markt-asf added a commit that referenced this pull request Feb 10, 2022
Fall back to the class loader used to load JULI when the thread context
class loader is not set. In a normal Tomcat configuration, this will be
the system class loader.
Based on a pull request by jackshirazi.
markt-asf added a commit that referenced this pull request Feb 10, 2022
Fall back to the class loader used to load JULI when the thread context
class loader is not set. In a normal Tomcat configuration, this will be
the system class loader.
Based on a pull request by jackshirazi.
markt-asf added a commit that referenced this pull request Feb 10, 2022
Fall back to the class loader used to load JULI when the thread context
class loader is not set. In a normal Tomcat configuration, this will be
the system class loader.
Based on a pull request by jackshirazi.
markt-asf added a commit that referenced this pull request Feb 10, 2022
Fall back to the class loader used to load JULI when the thread context
class loader is not set. In a normal Tomcat configuration, this will be
the system class loader.
Based on a pull request by jackshirazi.
@markt-asf

Copy link
Copy Markdown
Contributor

Broader patch with refactoring applied. Credit given for your inspiration.

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