Skip to content
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

ClassLoaderContextSelector should create a separate context per classloader #2314

Open
ppkarwasz opened this issue Feb 22, 2024 · 2 comments
Labels
bug Incorrect, unexpected, or unintended behavior of existing code

Comments

@ppkarwasz
Copy link
Contributor

The ClassLoaderContextSelector#locateContext currently implements the following logic:

if (ref == null) {
if (configLocation == null) {
ClassLoader parent = loader.getParent();
while (parent != null) {
ref = CONTEXT_MAP.get(toContextMapKey(parent));
if (ref != null) {
final WeakReference<LoggerContext> r = ref.get();
final LoggerContext ctx = r.get();
if (ctx != null) {
return ctx;
}
}
parent = parent.getParent();
/* In Tomcat 6 the parent of the JSP classloader is the webapp classloader which would be
configured by the WebAppContextListener. The WebAppClassLoader is also the ThreadContextClassLoader.
In JBoss 5 the parent of the JSP ClassLoader is the WebAppClassLoader which is also the
ThreadContextClassLoader. However, the parent of the WebAppClassLoader is the ClassLoader
that is configured by the WebAppContextListener.
ClassLoader threadLoader = null;
try {
threadLoader = Thread.currentThread().getContextClassLoader();
} catch (Exception ex) {
// Ignore SecurityException
}
if (threadLoader != null && threadLoader == parent) {
break;
} else {
parent = parent.getParent();
} */
}
}

If the initial ContextSelector#getContext call does not provide a configLocation parameter and there is already a logger context associated to an ancestor of the loader classloader, no new context is created and the logger context of the ancestor is returned.

While this might be a feature to minimize the number of logger contexts, IMHO it should be optional and the default behavior should be to create a different logger context per classloader.

This behavior causes #1430 and the problems in #2311.

@ppkarwasz ppkarwasz added the bug Incorrect, unexpected, or unintended behavior of existing code label Feb 22, 2024
@ppkarwasz
Copy link
Contributor Author

@rgoers, what do you think about removing the code above or at least activate it only if a property is set?

@ppkarwasz
Copy link
Contributor Author

As I see it, the main problem to solve is: if getContext is called for a classloader loader, for which no logger context exists, should ClassLoaderContextSelector return a new logger context or the context for loader.getParent()?

There are obviously 3 solutions to this problem

  • always return the logger context for loader.getParent() (if it exists, etc.). In practice this is the current behavior.
  • always return a new logger context. This solution causes logger context proliferation, even for classloaders where it is not necessary (e.g. they have the same configuration as their parent classloader). However it is not a big problem: since most Log4j Core appenders rely on a reference counted manager 75 file appenders pointing to the same file will still only use one file manager.
  • the right balance between the two above.

The right balance might be tricky to achieve: the same configuration file can, due to lookups and arbiters, create two totally different configurations, therefore I would prefer to always create a new logger context unconditionally.

However, if we are looking for a middle way, the one used by Tomcat JULI seems viable: Tomcat JULI uses the same logger context for all applications, except those for which loader.findResource("WEB-INF/logging.properties") is not null (not the recursive getResource).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect, unexpected, or unintended behavior of existing code
Projects
None yet
Development

No branches or pull requests

1 participant