[go: up one dir, main page]

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

Classes should no longer be specified via system properties #1977

Open
jvz opened this issue Nov 17, 2023 · 3 comments
Open

Classes should no longer be specified via system properties #1977

jvz opened this issue Nov 17, 2023 · 3 comments
Assignees
Labels
api Affects the public API configuration Affects the configuration system in a general way
Milestone

Comments

@jvz
Copy link
Member
jvz commented Nov 17, 2023

Over time, we collected many places in the codebase that accept an override by specifying a class name through a system property. These typically worked by guessing at an appropriate ClassLoader to use to load the class and then using reflection to instantiate it. This pattern is problematic in module systems like the Java module system and OSGi due to stronger encapsulation guarantees. As we've already implemented for plugins, anything else remaining that customizes class selection based on system properties should be updated to use java.util.ServiceLoader (when customizing in log4j-api) or specified in bundle classes installed via a service loader (which is also where some of the old system properties customizations are currently supported and can be simplified).

In general, the goal of this is to eliminate the use of LoaderUtil::load and any other implicit ClassLoader usage.

For some concrete examples, as I've worked on this, I've found the following interfaces that can be specified by system properties which are generally being refactored into what I said above.

Log4j API

The following interfaces are being loaded through service loader provider classes. These are similar in a way to the previous iteration of the Provider service class which grouped some of these things together.

  • org.apache.logging.log4j.spi.LoggerContextFactory
  • org.apache.logging.log4j.spi.ThreadContextMap
  • org.apache.logging.log4j.spi.ThreadContextStack
  • org.apache.logging.log4j.message.MessageFactory
  • org.apache.logging.log4j.message.FlowMessageFactory

There are some other service loader classes in the API, but they're either new or were already using service loading.

Log4j Core

The following interfaces either allow for overriding a default using a system property or are only configured through a system property. These should be updated to use bindings (or service loaders if that's not possible).

  • com.lmax.disruptor.ExceptionHandler<AsyncLoggerConfigDisruptor.Log4jEventWrapper> - AsyncLogger-specific exception handler
  • com.lmax.disruptor.ExceptionHandler<RingBufferLogEvent> - AsyncLoggerContext-specific exception handler
  • org.apache.logging.log4j.core.async.AsyncQueueFullPolicy
  • org.apache.logging.log4j.core.async.AsyncWaitStrategyFactory
  • org.apache.logging.log4j.core.util.AuthorizationProvider
  • org.apache.logging.log4j.core.time.Clock
  • org.apache.logging.log4j.core.config.ConfigurationFactory
  • org.apache.logging.log4j.core.config.ReliabilityStrategy
  • org.apache.logging.log4j.core.ContextDataInjector
  • Reflective instantiation of a org.apache.logging.log4j.util.StringMap for ContextDataFactory

Other

There are a few standalone modules that use LoaderUtil::loadClass for things that can be ported to use DI directly. There are some other places still using LoaderUtil or Loader that should be cleaned up alongside all this.

@jvz jvz added the configuration Affects the configuration system in a general way label Nov 17, 2023
@jvz jvz added this to the 3.0.0 milestone Nov 17, 2023
@jvz jvz self-assigned this Nov 17, 2023
jvz added a commit to jvz/logging-log4j2 that referenced this issue Nov 18, 2023
jvz added a commit to jvz/logging-log4j2 that referenced this issue Nov 18, 2023
This begins breaking up the log4j SPI into discrete service providers.

Related to apache#1977.
@jvz jvz added the api Affects the public API label Nov 20, 2023
jvz added a commit to jvz/logging-log4j2 that referenced this issue Nov 20, 2023
jvz added a commit to jvz/logging-log4j2 that referenced this issue Nov 20, 2023
This begins breaking up the log4j SPI into discrete service providers.

Related to apache#1977.
jvz added a commit to jvz/logging-log4j2 that referenced this issue Nov 20, 2023
jvz added a commit to jvz/logging-log4j2 that referenced this issue Nov 20, 2023
This begins breaking up the log4j SPI into discrete service providers.

Related to apache#1977.
@jvz
Copy link
Member Author
jvz commented Nov 30, 2023

So far, I've managed to migrate most of these things. I had to disable some tests for now, though, as they significantly abuse internal details of the legacy way of doing things in order to work.

@jvz
Copy link
Member Author
jvz commented Nov 30, 2023

Although some of them might work properly after I try to figure out yet another architecture for the JUnit 5 extensions since splitting them up into multiple annotations doesn't seem to work too well for some reason.

@jvz
Copy link
Member Author
jvz commented Jan 16, 2024

Extracted #2204 from work done in previous attempt at this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API configuration Affects the configuration system in a general way
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

1 participant