-
Notifications
You must be signed in to change notification settings - Fork 41.6k
Simplify Log4J2LoggingSystem
#47424
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
base: main
Are you sure you want to change the base?
Simplify Log4J2LoggingSystem
#47424
Conversation
This change leverages the major Spring Boot version bump to streamline and harden `Log4J2LoggingSystem` in two key areas: ### 1. Association with `LoggerContext` Previously, each method fetched the `LoggerContext` directly from `LogManager` and cast it to `o.a.l.l.core.LoggerContext`. This approach introduced several issues: * **`ClassCastException` risks**: * When Log4j Core is on the classpath but not the active implementation (e.g. when `log4j-to-slf4j` is used). * During shutdown, when `LogManager` may return a `SimpleLoggerContext` (see spring-projects#26953). * **Unexpected reinitialization**: If the logger context had already been stopped, `Log4J2LoggingSystem` would trigger creation of a **new** context, even mid-shutdown. ### 2. Configuration format detection Configuration file detection was previously hardcoded in `Log4J2LoggingSystem`, which limited flexibility: * Harder to support additional configuration formats. * Coupled Spring Boot to internal Log4j Core classes such as `AuthorizationProvider`. This change now delegates configuration resolution to Log4j Core via: `ConfigurationFactory.getConfiguration(LoggerContext, String, URI, ClassLoader)` This reduces reliance on internal APIs and allows Log4j Core to handle configuration formats and factories more naturally. ### Summary * Avoids fragile casts and unintended logger context reinitializations. * Delegates configuration handling to Log4j Core for improved extensibility and reduced internal API usage. Signed-off-by: Piotr P. Karwasz <[email protected]>
You need to use Java 24 (or ideally 25) for NullAway to work correctly due to javac bugs in Java 23 and earlier. |
Thanks, I'll try JDK 25 to fix the failures reported by the CI and add some unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In loadOverride(...), I see a FileNotFoundException being caught and returned null, but there might be other I/O errors upstream. Should we guard more broadly or document the exception semantics?
This commit updates the test suite to align with the recent `Log4J2LoggingSystem` changes: * Each test now uses its own `LoggerContext`, making manual reset of previous state unnecessary. * Removes tests for `getStandardConfigLocation()` and `getSpringConfigLocation()`, as these methods are now deprecated no-ops and no longer attempt to infer configuration files based on classpath contents. * Removes a test for `UrlConfigurationFactory`, which is no longer in use. Signed-off-by: Piotr P. Karwasz <[email protected]>
6ad76ec
to
4fa55de
Compare
…log4j2-logging-system
Fixes `Log4j2RuntimeHints` after refactor. Signed-off-by: Piotr P. Karwasz <[email protected]>
Signed-off-by: Piotr P. Karwasz <[email protected]>
Signed-off-by: Piotr P. Karwasz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I was able to get a successful build running with JDK 24. (With JDK 25, a component in buildSrc
still fails.)
The PR should now be ready for review. While debugging some test failures, I noticed an interesting behavior:
- The Gradle tests don’t appear to use the standard
testRuntimeClasspath
(i.e., artifacts from~/.gradle/caches
), but instead resolve dependencies from the local Maven repository (~/.m2/repository
). This is likely due to Spring’s extensive customizations of the build environment. - More concerningly, the tests are pulling in Log4j 2.24.3 rather than the expected 2.25.2.
@Test | ||
void nonFileUrlsAreResolvedUsingLog4J2UrlConnectionFactory() { | ||
this.loggingSystem.beforeInitialize(); | ||
assertThatIllegalStateException() | ||
.isThrownBy(() -> this.loggingSystem.initialize(this.initializationContext, | ||
"http://localhost:8080/shouldnotwork", null)) | ||
.havingCause() | ||
.isInstanceOf(ProtocolException.class) | ||
.withMessageContaining("http has not been enabled"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is no longer useful, as the expected exception is now handled internally by Log4j Core’s ConfigurationFactory
, which returns null
instead. The specific cause of the failure is logged to the status logger.
<Configuration> | ||
<SpringProfile name="production, test"> | ||
<Loggers> | ||
<Loggers> | ||
<SpringProfile name="production, test"> | ||
<Logger name="org.springframework.boot.logging.log4j2" level="TRACE" /> | ||
</Loggers> | ||
</SpringProfile> | ||
</SpringProfile> | ||
</Loggers> | ||
</Configuration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this test deserve some explanation.
Previously, the test did not fail, but it was conceptually flawed. When the log4j2-test.xml
file was merged with this one, the resulting configuration looked like this:
<Configuration>
<Loggers>
...
</Loggers>
<SpringProfile>
<Loggers>
...
</Loggers>
</SpringProfile>
</Configuration>
After SpringProfile
evaluation, the resulting configuration becomes invalid because it defines two separate <Loggers>
elements. This leads to undefined behavior: currently, only the second <Loggers>
section is applied.
Thanks! I kept the same caught exception as in the previous code, but you’re absolutely right: other exceptions can occur while loading an optional configuration. In commit df9bd55, I extended the catch block to include:
|
I don't think the catch block should be changed, as it was initially added to ignore FileNotFoundException, which, in the case of optional, is semantically correct. Catching a broader exception breaks that semantics, meaning that if the file content is incorrect, an exception will no longer be thrown. Even more, if something is wrong with the file, returning just null completely hides the issue. |
This change leverages the major Spring Boot version bump to streamline and harden
Log4J2LoggingSystem
in two key areas:1. Association with
LoggerContext
Previously, each method fetched the
LoggerContext
directly fromLogManager
and cast it too.a.l.l.core.LoggerContext
. This approach introduced several issues:ClassCastException
risks:log4j-to-slf4j
is used).LogManager
may return aSimpleLoggerContext
(see SpringApplicationShutdownHook throws ClassCastException when use log4j2 #26953).Unexpected reinitialization: If the logger context had already been stopped,
Log4J2LoggingSystem
would trigger creation of a new context, even mid-shutdown.2. Configuration format detection
Configuration file detection was previously hardcoded in
Log4J2LoggingSystem
, which limited flexibility:AuthorizationProvider
.This change now delegates configuration resolution to Log4j Core via:
ConfigurationFactory.getConfiguration(LoggerContext, String, URI, ClassLoader)
This reduces reliance on internal APIs and allows Log4j Core to handle configuration formats and factories more naturally.
Summary
Notes
loadConfiguration
methods to enable further refactorings in Spring Boot 4.1.x. For example, once Add getConfiguration method for multiple URIs apache/logging-log4j2#3921 is included in Log4j2.26.0
, configuration merging could also be delegated to Log4j Core, further reducing Spring Boot’s dependency surface on internal Log4j Core APIs.