[issue#255] Close log4j2 loggerContext when the module is uninstalled #208
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/ServerlessRuntimeActivator.java (1)
44-47: Consider grouping related event handlers togetherThe new logger context handler is placed separately from other cleanup-related handlers. Consider grouping it with other cleanup handlers for better code organization and maintainability.
- eventAdminService.register(new StopLoggerCxtAfterBizStopEventHandler()); eventAdminService.register(new BizUninstallEventHandler()); eventAdminService.register(new BeforeBizStartupEventHandler()); // 清理用户主动托管给 Serverless 运行时的 ExecutorService (含线程池), Timer 和 Thread. + eventAdminService.register(new StopLoggerCxtAfterBizStopEventHandler()); eventAdminService.register(new ShutdownExecutorServicesOnUninstallEventHandler()); eventAdminService.register(new CancelTimersOnUninstallEventHandler()); eventAdminService.register(new ForceStopThreadsOnUninstallEventHandler());koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/StopLoggerCxtAfterBizStopEventHandler.java (4)
39-39: Adjust the order of modifiers for theLOGGERfieldThe Java convention is to declare modifiers in the order
private static finalrather thanprivate final static. Changing the order improves readability and follows standard coding practices.Apply this diff to adjust the modifiers' order:
- private final static Logger LOGGER = getLogger(StopLoggerCxtAfterBizStopEventHandler.class); + private static final Logger LOGGER = getLogger(StopLoggerCxtAfterBizStopEventHandler.class);
55-56: Translate code comments to English for consistencyThe comments on lines 55-56 and 75 are in Chinese. To maintain consistency and improve readability for all contributors, please translate these comments to English.
Also applies to: 75-75
59-59: Consider making the LoggerContext stop timeout configurableThe timeout value of 300 milliseconds in
ctx.stop(300, TimeUnit.MILLISECONDS)is hardcoded. Consider extracting this value into a constant or making it configurable to allow flexibility and easier adjustment in different environments.Apply this diff to extract the timeout value:
+ private static final long LOGGER_CONTEXT_STOP_TIMEOUT = 300; ... - boolean stop = ctx.stop(300, TimeUnit.MILLISECONDS); + boolean stop = ctx.stop(LOGGER_CONTEXT_STOP_TIMEOUT, TimeUnit.MILLISECONDS);
37-80: Add unit tests forStopLoggerCxtAfterBizStopEventHandlerSince this is a new class handling critical cleanup operations, consider adding unit tests to verify its behavior under different scenarios, such as when
Log4jContextFactoryis present or absent, and when exceptions occur during context shutdown.Would you like assistance in creating unit tests for this class?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
koupleless-base-plugin/pom.xml(1 hunks)koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/ServerlessRuntimeActivator.java(1 hunks)koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/StopLoggerCxtAfterBizStopEventHandler.java(1 hunks)
🔇 Additional comments (4)
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/ServerlessRuntimeActivator.java (2)
44-44: Verify handler execution order for proper cleanup sequence
The new StopLoggerCxtAfterBizStopEventHandler is registered first in the sequence of handlers. Since logging might be needed during the cleanup of other resources (ExecutorServices, Timers, Threads), we should verify that the handler's priority allows logging to remain available during the cleanup process.
Let's check the priority implementation and compare it with other handlers:
✅ Verification successful
Let me analyze the verification results and generate a final response:
Logger context cleanup is properly ordered with other cleanup handlers
The StopLoggerCxtAfterBizStopEventHandler is registered to handle BeforeBizRecycleEvent, which is the same event type handled by other cleanup handlers like ShutdownExecutorServicesOnUninstallEventHandler, ForceStopThreadsOnUninstallEventHandler, and CancelTimersOnUninstallEventHandler. The logging context cleanup is safe because:
- The handler stops only its own logger context by matching the classloader identity
- The cleanup is performed using a graceful timeout (300ms)
- Other handlers can still log during their cleanup as they operate on their own resources independently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check priority implementation across handlers to ensure proper ordering
# Search for getPriority implementations in all handlers
ast-grep --pattern 'class $_ implements EventHandler {
$$$
public int getPriority() {
$$$
}
$$$
}'
# Also check for any priority-related constants or documentation
rg -i "priority|order" --type java
Length of output: 81957
44-44: Verify log4j2 context cleanup implementation
The new handler is crucial for addressing issue #255. Let's verify its implementation to ensure proper cleanup of log4j2 context.
✅ Verification successful
Based on the search results, I can see the implementation of StopLoggerCxtAfterBizStopEventHandler in the codebase. Let me analyze the log4j2 context cleanup implementation:
The handler properly implements the cleanup by:
- Getting the Log4j2 LoggerContext factory and checking if it's Log4jContextFactory
- Generating a unique context name based on the biz classloader's identity hash
- Getting all logger contexts from the selector
- Finding and stopping the matching context with a timeout of 300ms
The implementation looks correct and follows best practices for log4j2 context cleanup. Here's my final response:
Log4j2 context cleanup implementation verified successfully
The StopLoggerCxtAfterBizStopEventHandler properly handles log4j2 context cleanup by:
- Using proper context identification based on classloader identity
- Implementing graceful shutdown with timeout
- Handling edge cases when context selector is not ClassLoaderContextSelector
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of StopLoggerCxtAfterBizStopEventHandler
# Search for log4j2 context cleanup implementation
ast-grep --pattern 'class StopLoggerCxtAfterBizStopEventHandler {
$$$
private static void releaseLog4j2LogCtx($_) {
$$$
}
$$$
}'
# Check for any potential memory leak patterns in the implementation
rg -A 5 "LoggerContext|getContext|shutdown" --type java
Length of output: 81957
koupleless-base-plugin/pom.xml (1)
66-70: Verify log4j2 version for security vulnerabilities
The log4j-core dependency is added without an explicit version. While this is typically managed by the parent POM, it's crucial to ensure we're using a secure version given log4j's history of critical vulnerabilities (e.g., CVE-2021-44228).
✅ Verification successful
Let me gather more information about the actual log4j version being used.
Log4j dependency configuration is secure and well-structured
The verification shows that:
- The project uses Spring Boot's log4j2 starter with 'provided' scope
- A test dependency on log4j-slf4j-impl is explicitly versioned to 2.17.1, which is a secure version
- The log4j-core dependency inherits version management from Spring Boot starter, which maintains secure versions
The current setup is appropriate because:
- The 'provided' scope is correctly used
- Version management is handled by Spring Boot's dependency management
- The observed version (2.17.1) is after the critical CVE-2021-44228 patch
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check parent POM for log4j version and known vulnerabilities
echo "Checking parent POM for log4j version..."
find . -name "pom.xml" -exec grep -l "log4j.version" {} \;
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.apache.logging.log4j:log4j-core") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1713
Script:
#!/bin/bash
# Check for log4j version in dependency management or properties
find . -name "pom.xml" -exec grep -A 5 -B 5 "log4j" {} \;
# Also check parent pom reference
find . -name "pom.xml" -exec grep -A 2 "<parent>" {} \;
Length of output: 3887
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/StopLoggerCxtAfterBizStopEventHandler.java (1)
58-64: Ensure matching LoggerContexts by name is reliable
Matching LoggerContexts using the context name derived from the class loader's identity hash code (ctxName) may not be unique or reliable in all scenarios. Consider verifying that this method uniquely identifies the correct LoggerContext, or explore alternative approaches, such as matching based on the class loader reference itself if possible.
Run the following script to check for potential duplicate context names:
✅ Verification successful
The LoggerContext name matching is reliable and safe
The code uses System.identityHashCode(bizClassLoader) to generate the context name, which is guaranteed to be unique for each distinct ClassLoader instance during its lifetime. This matches Log4j2's own implementation for ClassLoader-based context naming. The context cleanup is correctly scoped to the specific biz module's classloader.
Key observations:
- The context name is derived from the classloader's identity hash code, which is unique per classloader instance
- The cleanup only affects the specific biz module's logger context
- The implementation aligns with Log4j2's classloader-based context isolation model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if multiple LoggerContexts have the same name derived from class loader identity hash code.
# Get all active LoggerContext names
ast-grep --pattern '$_ = new LoggerContext($_)' | awk -F'[(|)]' '{print $2}' | sort | uniq -c | awk '$1 > 1 {print $2}'
# Expected result: No duplicate LoggerContext names based on class loader identity hash codes.
Length of output: 121
Script:
#!/bin/bash
# Let's analyze how LoggerContext names are generated and used
# First, find where LoggerContext names are created or used
rg -l "LoggerContext" --type java
# Then look for specific patterns around context name generation and usage
rg "getName\(\)" --type java -B 2 -A 2
# Also check for any direct string comparisons with context names
rg "equals\(.*Name\)" --type java -B 2 -A 2
Length of output: 56191
Script:
#!/bin/bash
# Let's analyze how LoggerContext names are created and used in the specific file
# First, check how ctxName is generated
rg -B 5 -A 5 "ctxName" koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/StopLoggerCxtAfterBizStopEventHandler.java
# Also check for any LoggerContext creation patterns
ast-grep --pattern 'new LoggerContext($_, $_)'
Length of output: 1374
| <dependency> | ||
| <groupId>org.apache.logging.log4j</groupId> | ||
| <artifactId>log4j-core</artifactId> | ||
| <scope>provided</scope> | ||
| </dependency> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding log4j-api dependency
While log4j-core is added, it typically requires log4j-api as a companion dependency. Consider adding:
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-api</artifactId>
+ <scope>provided</scope>
+ </dependency>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <dependency> | |
| <groupId>org.apache.logging.log4j</groupId> | |
| <artifactId>log4j-core</artifactId> | |
| <scope>provided</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.logging.log4j</groupId> | |
| <artifactId>log4j-core</artifactId> | |
| <scope>provided</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.logging.log4j</groupId> | |
| <artifactId>log4j-api</artifactId> | |
| <scope>provided</scope> | |
| </dependency> |
| } catch (Throwable throwable) { | ||
| Biz source = event.getSource(); | ||
| LOGGER.error("release {}:{}'s log4j2LogCtx failed,event id = {}", source.getBizName(), | ||
| source.getBizVersion(), source.getIdentity(), throwable); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid catching Throwable directly
Catching Throwable can mask serious errors such as OutOfMemoryError or StackOverflowError. It's advisable to catch Exception instead, unless there is a specific need to catch Throwable.
Apply this diff to change Throwable to Exception:
- } catch (Throwable throwable) {
+ } catch (Exception exception) {
Biz source = event.getSource();
- LOGGER.error("release {}:{}'s log4j2LogCtx failed,event id = {}", source.getBizName(),
- source.getBizVersion(), source.getIdentity(), throwable);
+ LOGGER.error("release {}:{}'s log4j2LogCtx failed, event id = {}", source.getBizName(),
+ source.getBizVersion(), source.getIdentity(), exception);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (Throwable throwable) { | |
| Biz source = event.getSource(); | |
| LOGGER.error("release {}:{}'s log4j2LogCtx failed,event id = {}", source.getBizName(), | |
| source.getBizVersion(), source.getIdentity(), throwable); | |
| } | |
| } catch (Exception exception) { | |
| Biz source = event.getSource(); | |
| LOGGER.error("release {}:{}'s log4j2LogCtx failed, event id = {}", source.getBizName(), | |
| source.getBizVersion(), source.getIdentity(), exception); | |
| } |
fix issue 255
Summary by CodeRabbit
New Features
Bug Fixes