This is exactly why we love getting new committers on board!

When I wrote this code my thinking was that I have no idea what ClassLoader 
scheme the app is running in. Heck, JBoss had at least 3 of them in the time 
I worked with it years ago.  So the idea here is to simply find implementations 
wherever they can be found and use the ones that work. I know it isn’t pretty 
but I couldn’t figure out a better way. This is one of the major benefits of 
JPMS 
is that it definitely cleans this up.

I have no idea what is going on with item 4.

Ralph

> On Mar 18, 2022, at 2:50 AM, Piotr P. Karwasz <piotr.karw...@gmail.com> wrote:
> 
> Hello,
> 
> I am trying to understand the way the context classloader and
> `ServiceLoader` are used in Log4j 2.x[1].
> 
> The `log4j-api` uses `LoaderUtil.getClassLoaders` in several places to
> lookup services from the context classloader and the `log4j-api`
> classloader.
> 
> This procedure has IMHO some problems:
> 1. The services retrieved this way are assigned to static fields
> (except the call in `WatchEventService`): this causes a memory leak if
> `log4j-api` is in the server's classloader and the retrieved service
> in the web application classloader,
> 
> 2. There is the small problem that if any service found by
> `ServiceLoader.load(Class, ClassLoader)` fails the `instanceof` check,
> all the others are thrown away. I solved it in `ServiceLoaderUtil`,
> 
> 3. Probably this is not a problem, but I fail to understand why we
> look up services separately for a classloader and each of its
> ancestors.
>  If the classloader implements the "delegate to parent" convention,
> we'll end up several instances of the same class.
>   If the classloader is a webapp classloader and the service
> interface is in the webapp classloader, all services in the
> classloader ancestors will fail the 'instanceof' test.
>   The only situation where this makes sense is if the service
> interface is in the server classloader (e.g. `javax.sql.DataSource`),
> then we can have two different services with the same class name. Is
> this the reason?
> 
> 4. The following condition in `ThreadContextDataInjector` is pure magic for 
> me:
>        if (providers.stream().noneMatch(p ->
> p.getClass().isAssignableFrom(provider.getClass()))) {
>            providers.add(provider);
> }
>   As I understand it, if we have a base class, we don't want its
> derived classes?
> 
> I have don't some refactoring of the ServiceLoader code in:
> https://github.com/apache/logging-log4j2/pull/804
> 
> 
> [1] https://issues.apache.org/jira/browse/LOG4J2-3427

Reply via email to