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