Eliminate ClassUnload during config reload using Configuration.initialize() #14025
Conversation
…lize() Fixes alibaba#13940 - Apply official PR alibaba#14000 fix to v2.x-develop branch This commit applies the official fix from alibaba/nacos PR alibaba#14000 (commit 90747d4) to resolve ClassUnload issues during Log4j2 configuration reload. Key changes: 1. Add NacosLog4j2Configurator class using Configuration.initialize() instead of Configuration.start() to avoid plugin reinitialization 2. Refactor Log4J2NacosLoggingAdapter to delegate to new configurator 3. Remove deprecated helper methods and simplify test implementation 4. Add thread-safe double-checked locking pattern 5. Additively merge Nacos config without replacing user's logging setup The fix follows framework-compliant pattern similar to Logback adapter. Adapted from: alibaba#14000 Original commit: 90747d4
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #13940 where Log4j2 configuration reload causes ClassUnload in middleware environments (Dubbo, Spring Cloud, etc.). The root cause is that calling Configuration.start() reinitializes Log4j2 plugins, triggering class unloading of Nacos classes. The fix uses Configuration.initialize() instead, which sets up the configuration without plugin reinitialization. This implementation follows the same additive configuration pattern as the existing Logback adapter (NacosLogbackConfiguratorAdapterV1).
Key Changes:
- Introduced
NacosLog4j2Configuratorto handle Log4j2 configuration usingConfiguration.initialize()instead ofConfiguration.start() - Refactored
Log4J2NacosLoggingAdapterto delegate to the new configurator with thread-safe double-checked locking - Updated tests to remove Mockito dependencies and verify functionality works with the new implementation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
NacosLog4j2Configurator.java |
New configurator class that uses Configuration.initialize() to avoid ClassUnload issue and additively merges Nacos logging configuration |
Log4J2NacosLoggingAdapter.java |
Refactored to use NacosLog4j2Configurator with thread-safe double-checked locking pattern for configuration loading |
Log4j2NacosLoggingPropertiesHolder.java |
Added public getter method for properties (currently unused) |
Log4J2NacosLoggingAdapterTest.java |
Removed Mockito-based tests and verification calls to align with v2.x testing approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static NacosLoggingProperties getProperties() { | ||
| return INSTANCE.properties; | ||
| } |
There was a problem hiding this comment.
The newly added getProperties() method is not used anywhere in the codebase. If it's intended for future use or as a public API, consider documenting its purpose. If it's not needed, consider removing it to avoid maintaining unused code.
| public static NacosLoggingProperties getProperties() { | |
| return INSTANCE.properties; | |
| } |
| public class NacosLog4j2Configurator { | ||
|
|
||
| private static final String NACOS_LOGGER_PREFIX = "com.alibaba.nacos"; | ||
|
|
||
| /** | ||
| * Configure LoggerContext by loading Nacos configuration from URI. | ||
| * This method additively merges Nacos appenders and loggers into the existing configuration. | ||
| * | ||
| * @param loggerContext The LoggerContext to configure | ||
| * @param configLocation URI of the Nacos Log4j2 configuration file | ||
| * @throws IOException if configuration file cannot be read | ||
| */ | ||
| public void configure(LoggerContext loggerContext, URI configLocation) throws IOException { | ||
| Configuration nacosConfig = loadConfiguration(loggerContext, configLocation); | ||
|
|
||
| // Key fix for issue #13940: Use initialize() instead of start() | ||
| // initialize() sets up the configuration without triggering plugin reinitialization | ||
| nacosConfig.initialize(); | ||
|
|
||
| // Get the current active configuration | ||
| Configuration currentConfig = loggerContext.getConfiguration(); | ||
|
|
||
| // Additively merge Nacos appenders (non-invasive approach for middleware) | ||
| // Note: Appenders are started individually and added to currentConfig | ||
| // They are NOT removed from nacosConfig to avoid lifecycle issues | ||
| nacosConfig.getAppenders().values().forEach(appender -> { | ||
| if (!appender.isStarted()) { | ||
| appender.start(); | ||
| } | ||
| currentConfig.addAppender(appender); | ||
| }); | ||
|
|
||
| // Add only Nacos-specific loggers to avoid interfering with user configuration | ||
| nacosConfig.getLoggers().entrySet().stream() | ||
| .filter(entry -> entry.getKey().startsWith(NACOS_LOGGER_PREFIX)) | ||
| .forEach(entry -> currentConfig.addLogger(entry.getKey(), entry.getValue())); | ||
|
|
||
| // Apply the merged configuration | ||
| loggerContext.updateLoggers(); | ||
|
|
||
| // Important: Do NOT call nacosConfig.stop() here! | ||
| // The appenders and loggers have been transferred to currentConfig. | ||
| // Calling stop() would shut down the appenders that are now owned by currentConfig. | ||
| // nacosConfig will be garbage collected naturally, and since we only called initialize() | ||
| // (not start()), there are no active background threads or resources to clean up. | ||
| } | ||
|
|
||
| /** | ||
| * Load Log4j2 configuration from URI using ConfigurationFactory. | ||
| * This is the standard Log4j2 way to parse configuration files. | ||
| * | ||
| * @param ctx LoggerContext | ||
| * @param configLocation URI of configuration file | ||
| * @return Parsed Configuration object | ||
| * @throws IOException if configuration cannot be loaded | ||
| */ | ||
| private Configuration loadConfiguration(LoggerContext ctx, URI configLocation) throws IOException { | ||
| try (InputStream stream = configLocation.toURL().openStream()) { | ||
| ConfigurationSource source = new ConfigurationSource(stream, configLocation.toURL()); | ||
| return ConfigurationFactory.getInstance().getConfiguration(ctx, source); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new NacosLog4j2Configurator class introduced in this PR lacks direct unit tests. While it's indirectly tested through Log4J2NacosLoggingAdapterTest, consider adding dedicated unit tests to verify its behavior in isolation, particularly the additive configuration merging logic and the use of Configuration.initialize() vs Configuration.start().
1 participant
[ISSUE #13940] Eliminate ClassUnload during config reload using Configuration.initialize()
What is the purpose of the change
This PR fixes issue #13940 where Log4j2 configuration reload causes ClassUnload in middleware environments (Dubbo, Spring Cloud, etc.). The root cause is using
Configuration.start()which reinitializes Log4j2 plugins, triggering class unloading of Nacos classes. By usingConfiguration.initialize()instead, we avoid plugin reinitialization while properly configuring Log4j2.Brief changelog
NacosLog4j2Configuratorclass that usesConfiguration.initialize()instead ofConfiguration.start()to avoid plugin reinitialization and ClassUnload issuesLog4J2NacosLoggingAdapterto delegate configuration loading to the newNacosLog4j2ConfiguratorVerifying this change
Test Results
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
Files Changed
logger-adapter-impl/log4j2-adapter/src/main/java/com/alibaba/nacos/logger/adapter/log4j2/NacosLog4j2Configurator.java(NEW - 109 lines)logger-adapter-impl/log4j2-adapter/src/main/java/com/alibaba/nacos/logger/adapter/log4j2/Log4J2NacosLoggingAdapter.javalogger-adapter-impl/log4j2-adapter/src/main/java/com/alibaba/nacos/logger/adapter/log4j2/Log4j2NacosLoggingPropertiesHolder.javalogger-adapter-impl/log4j2-adapter/src/test/java/com/alibaba/nacos/logger/adapter/log4j2/Log4J2NacosLoggingAdapterTest.javaRelated Issues
Fixes #13940
Based on: #14000 (commit 90747d4)
Notes for Reviewers
This PR directly applies the official fix from alibaba/nacos PR #14000 to v2.x-develop branch. The implementation:
Configuration.initialize()instead ofConfiguration.start()configuration.stop()to preserve transferred appenders