Skip to content

avoid null classloader when thread context CL is null#464

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

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

Conversation

@jackshirazi
Copy link

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