[issue#255] feat: release log4j resource after biz uninstall #212
Conversation
WalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
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: 7
🧹 Outside diff range and nitpick comments (9)
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/releaser/ResourceReleaser.java (1)
5-11: Enhance interface documentation for better clarity.While the basic documentation is present, consider adding more detailed documentation to:
- Explain the interface's role in resource release lifecycle
- Specify contract requirements for implementers
- Document the relationship with business uninstallation
- Add
@sincetag for version tracking/** * @author: poorpaper * @time: 2024/11/24 00:16 + * @since 1.0.0 * * 通用资源释放接口 + * + * This interface defines a contract for releasing resources associated with a Biz instance + * during the uninstallation process. Implementations should handle cleanup of specific + * resources (e.g., Log4j resources) in a thread-safe manner. + * + * Implementing classes must ensure that: + * - Resource release is idempotent + * - All exceptions are properly handled + * - No resources are left in an inconsistent state */koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/releaser/Log4jResourceReleaser.java (3)
15-20: Enhance class documentation with implementation detailsThe current documentation is in Chinese and lacks crucial implementation details. Consider adding:
- English documentation
- Thread safety guarantees
- Explanation of the 300ms timeout value
- Potential impact of resource cleanup on the system
/** - * @author: poorpaper - * @time: 2024/11/24 00:27 - * - * Log4j2资源释放器 + * Log4j2 resource releaser responsible for cleaning up logger contexts + * associated with uninstalled business modules. + * + * @author poorpaper + * @since 2024/11/24 + * + * @implNote This implementation uses a 300ms timeout when stopping logger contexts. + * The cleanup process is thread-safe as Log4j2's internal synchronization is utilized. + * If cleanup fails, it will be logged but won't block the uninstallation process. */
32-33: Consider more robust context naming strategyUsing
identityHashCodealone might lead to naming conflicts in rare cases. Consider incorporating additional unique identifiers from theBizinstance.- String ctxName = Integer.toHexString(System.identityHashCode(bizClassLoader)); + String ctxName = String.format("%s_%s_%s", + biz.getBizName(), + biz.getBizVersion(), + Integer.toHexString(System.identityHashCode(bizClassLoader)));
42-45: Refine error handling approachThe current error handling catches
Throwablewhich is too broad and the error message could be more specific.- } catch (Throwable t) { - LOGGER.error(String.format("[Log4jResourceReleaser] Error happened when clean biz %s log4j cache", - biz.getIdentity()), t); + } catch (Exception e) { + LOGGER.error("[Log4jResourceReleaser] Failed to cleanup Log4j context for biz {}: {}", + biz.getIdentity(), e.getMessage(), e);koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/ServerlessRuntimeActivator.java (1)
47-50: Consider using consistent language in comments.The comment mixes Chinese and English. Consider using a single language throughout the codebase for better maintainability and international collaboration.
Suggested English version:
- // 清理用户主动托管给 Serverless 运行时的 ExecutorService (含线程池), Timer, Thread 与 资源(如Log4jContext). + // Cleanup of user-managed resources in Serverless runtime: ExecutorService (including thread pools), Timer, Thread, and resources (e.g., Log4jContext).koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResourceReleaseOnUninstallEventHandler.java (1)
57-60: Document priority selection rationaleThe handler is set to
LOWEST_PRECEDENCEwithout explanation. This could be problematic if:
- Other cleanup handlers depend on these resources
- Order of cleanup operations is critical
Please add documentation explaining:
- Why lowest priority was chosen
- Any dependencies or ordering requirements
@Override + /** + * Returns the handler's priority level. + * Set to LOWEST_PRECEDENCE because [explain rationale here] + * @return The priority level for this event handler + */ public int getPriority() { return LOWEST_PRECEDENCE; }koupleless-base-plugin/src/test/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResouceReleaseOnUninstallEventHandlerTest.java (3)
23-23: Fix typo in class name: "Resouce" should be "Resource"The class name contains a spelling error that should be corrected for consistency and clarity.
-public class ResouceReleaseOnUninstallEventHandlerTest { +public class ResourceReleaseOnUninstallEventHandlerTest {
35-36: Consider moving Log4j configuration to a test properties fileSetting system properties directly in the test code makes it harder to maintain and reuse configurations. Consider moving these to a test configuration file.
48-50: Simplify LoggerContext startup logicThe initialization check before starting the context seems redundant. The
start()method is idempotent and can be called directly.- if (testLoggerContext.isInitialized()) { - testLoggerContext.start(); - } + testLoggerContext.start();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
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/ResourceReleaseOnUninstallEventHandler.java(1 hunks)koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/releaser/Log4jResourceReleaser.java(1 hunks)koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/releaser/ResourceReleaser.java(1 hunks)koupleless-base-plugin/src/test/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResouceReleaseOnUninstallEventHandlerTest.java(1 hunks)koupleless-base-plugin/src/test/resources/log4j2-test.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- koupleless-base-plugin/src/test/resources/log4j2-test.xml
🔇 Additional comments (9)
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/releaser/ResourceReleaser.java (2)
1-4: LGTM! Package structure and imports are appropriate.
The package placement in the handler hierarchy and the import of Biz are well-organized.
12-20: 🛠️ Refactor suggestion
Improve method documentation and consider error handling.
The method signature is clean, but consider these improvements:
- Enhance method documentation
- Specify error handling expectations
- Consider adding a return value to indicate success/failure
public interface ResourceReleaser {
/**
- * 获取待释放资源上下文
+ * Releases resources associated with the given Biz instance.
*
- * @param biz
+ * @param biz The business module whose resources need to be released
+ * @throws IllegalArgumentException if biz is null
+ * @throws ResourceReleaseException if resource release fails
+ * @return boolean indicating whether the release was successful
*/
- void doRelease(Biz biz);
+ boolean doRelease(Biz biz);
}Let's verify if any implementations already exist:
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/releaser/Log4jResourceReleaser.java (1)
22-47: Verify integration with event system
The implementation looks solid, but we should verify its integration with the event system.
✅ Verification successful
Integration with event system is properly implemented
The Log4j resource releaser is well integrated into the event system:
- Automatically registered in
ResourceReleaseOnUninstallEventHandlerconstructor - Handler is registered with event admin service in
ServerlessRuntimeActivator - Executed during the
BeforeBizRecycleEventevent handling - Stored in a thread-safe concurrent map (
BIZ_CLASS_LOADER_TO_RELEASER)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how this releaser is registered and the order of execution
rg -l "ResourceReleaseOnUninstallEventHandler|ResourceReleaser" --type java
# Check for other resource releasers to understand the execution order
ast-grep --pattern 'class $_ implements ResourceReleaser'
# Look for event handler registration
rg -A 5 "ResourceReleaseOnUninstallEventHandler"
Length of output: 8133
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/ServerlessRuntimeActivator.java (2)
42-42: LGTM! Improved code formatting.
The indentation adjustment enhances readability by properly aligning the method chain.
51-51: Verify handler execution order.
The new ResourceReleaseOnUninstallEventHandler is registered after other cleanup handlers. Please verify that the handler's priority ensures correct resource cleanup order, especially in relation to other handlers that might depend on Log4j for logging during their cleanup.
✅ Verification successful
Let me generate another script to search for the handler implementation and its priority:
The previous search didn't reveal the handler implementations. Let's try to find the specific handler class and any priority-related code to verify the execution order.
Let me try one more search with a different pattern to locate the handler and priority-related code:
This should help us find the handler implementation and understand how priorities are managed in the event system.
Priority order is correctly implemented for resource cleanup
The ResourceReleaseOnUninstallEventHandler has LOWEST_PRECEDENCE priority, which is appropriate since:
CancelTimersOnUninstallEventHandlerruns first withHIGHEST_PRECEDENCEShutdownExecutorServicesOnUninstallEventHandlerruns next withHIGHEST_PRECEDENCE + 1- Other handlers run with
DEFAULT_PRECEDENCE - Resource cleanup handlers (including Log4j) run last with
LOWEST_PRECEDENCE
This ensures that Log4j resources remain available for other handlers that might need logging during their cleanup operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the priority implementation of all handlers to ensure correct execution order
# Search for getPriority implementations in all handlers
ast-grep --pattern 'class $_ implements EventHandler {
$$$
getPriority() {
$$$
}
$$$
}'
# Also check for any direct logging calls in other handlers
rg -A 2 "log\." --type java
Length of output: 20246
Script:
#!/bin/bash
# Search for ResourceReleaseOnUninstallEventHandler implementation
ast-grep --pattern 'class ResourceReleaseOnUninstallEventHandler {
$$$
}'
# Also search for any class implementing EventHandler with priority
ast-grep --pattern 'class $_ implements $_ {
$$$
public int getPriority() {
$$$
}
$$$
}'
Length of output: 180
Script:
#!/bin/bash
# Search for ResourceReleaseOnUninstallEventHandler in any context
rg -l "ResourceReleaseOnUninstallEventHandler"
# Search for the file content if found
rg -A 10 "class ResourceReleaseOnUninstallEventHandler"
# Search for any priority-related code in event handlers
rg -A 5 "getPriority|priority.*=|PRIORITY" --type java
Length of output: 12395
koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResourceReleaseOnUninstallEventHandler.java (1)
1-61: Verify integration with Log4jResourceReleaser and usage
Please ensure:
- The Log4jResourceReleaser implementation properly handles all Log4j resources
- The handler is properly registered in the event system
✅ Verification successful
Let me gather more information about the Log4jResourceReleaser implementation.
Integration verified and properly implemented
The verification confirms:
-
Log4jResourceReleaser correctly implements the ResourceReleaser interface and properly handles Log4j resources by:
- Safely stopping logger contexts associated with the module's ClassLoader
- Including timeout handling (300ms) and error handling
- Properly cleaning up Log4j cache
-
The handler is properly registered in ServerlessRuntimeActivator and tested:
- Registered via eventAdminService.register()
- Comprehensive test coverage verifying logger context lifecycle
- Proper cleanup of resources during module uninstallation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Log4jResourceReleaser implementation
echo "Checking Log4jResourceReleaser implementation..."
ast-grep --pattern 'class Log4jResourceReleaser implements ResourceReleaser {
$$$
}'
# Check handler registration
echo "Checking handler registration..."
rg -A 5 "ResourceReleaseOnUninstallEventHandler"
Length of output: 7657
Script:
#!/bin/bash
# Check Log4jResourceReleaser implementation
echo "Checking Log4jResourceReleaser implementation..."
fd "Log4jResourceReleaser.java" -x cat {}
# Check ResourceReleaser interface
echo -e "\nChecking ResourceReleaser interface..."
fd "ResourceReleaser.java" -x cat {}
# Check test implementation
echo -e "\nChecking test implementation..."
fd "ResouceReleaseOnUninstallEventHandlerTest.java" -x cat {}
Length of output: 6887
koupleless-base-plugin/src/test/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResouceReleaseOnUninstallEventHandlerTest.java (1)
36-36: Verify log4j2-test.xml configuration file
The test depends on "log4j2-test.xml" configuration file.
✅ Verification successful
Log4j2 configuration file is present and properly formatted
The log4j2-test.xml file exists at the correct location (koupleless-base-plugin/src/test/resources/) and contains a valid XML configuration with properly defined appenders and loggers for test execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the log4j2-test.xml exists and validate its format
fd -t f "log4j2-test.xml" -x sh -c '
echo "Found config file: {}"
if command -v xmllint > /dev/null; then
xmllint --noout "{}" && echo "XML format is valid"
else
cat "{}"
fi
'
Length of output: 2047
koupleless-base-plugin/pom.xml (2)
70-74: Review Disruptor version for updates.
The Disruptor library is pinned to version 3.4.2 (from 2019). Consider updating to the latest stable version for potential performance improvements and bug fixes.
Run the following script to check for the latest version:
66-69: Specify and verify Log4j version for security.
Given that this PR focuses on Log4j resource management, it's crucial to explicitly specify and verify the Log4j version for security considerations.
Run the following script to check for security advisories and latest versions:
| ClassLoader bizClassLoader = biz.getBizClassLoader(); | ||
| LoggerContextFactory factory = LogManager.getFactory(); | ||
| if (factory instanceof Log4jContextFactory) { |
There was a problem hiding this comment.
Add null check for bizClassLoader
The getBizClassLoader() call could potentially return null. Add a defensive check to prevent NPE.
public void doRelease(Biz biz) {
try {
ClassLoader bizClassLoader = biz.getBizClassLoader();
+ if (bizClassLoader == null) {
+ LOGGER.warn("[Log4jResourceReleaser] Skipping cleanup for biz {} as classloader is null",
+ biz.getIdentity());
+ return;
+ }
LoggerContextFactory factory = LogManager.getFactory();📝 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.
| ClassLoader bizClassLoader = biz.getBizClassLoader(); | |
| LoggerContextFactory factory = LogManager.getFactory(); | |
| if (factory instanceof Log4jContextFactory) { | |
| ClassLoader bizClassLoader = biz.getBizClassLoader(); | |
| if (bizClassLoader == null) { | |
| LOGGER.warn("[Log4jResourceReleaser] Skipping cleanup for biz {} as classloader is null", | |
| biz.getIdentity()); | |
| return; | |
| } | |
| LoggerContextFactory factory = LogManager.getFactory(); | |
| if (factory instanceof Log4jContextFactory) { |
| boolean stop = context.stop(300, TimeUnit.MILLISECONDS); | ||
| LOGGER.info("[Log4jResourceReleaser] Stop biz {}:{} logger context,result={}", | ||
| biz.getBizName(), biz.getBizVersion(), stop); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make timeout configurable and improve logging
The 300ms timeout is hardcoded and the logging could be more informative about timeout situations.
+ private static final long DEFAULT_TIMEOUT_MS = 300;
+ private final long timeoutMs;
+
+ public Log4jResourceReleaser(long timeoutMs) {
+ this.timeoutMs = timeoutMs > 0 ? timeoutMs : DEFAULT_TIMEOUT_MS;
+ }
+
+ public Log4jResourceReleaser() {
+ this(DEFAULT_TIMEOUT_MS);
+ }
// In doRelease method:
- boolean stop = context.stop(300, TimeUnit.MILLISECONDS);
+ boolean stop = context.stop(timeoutMs, TimeUnit.MILLISECONDS);
LOGGER.info("[Log4jResourceReleaser] Stop biz {}:{} logger context,result={}",
- biz.getBizName(), biz.getBizVersion(), stop);
+ biz.getBizName(), biz.getBizVersion(),
+ stop ? "success" : "timed out after " + timeoutMs + "ms");Committable suggestion skipped: line range outside the PR's diff.
| public ResourceReleaseOnUninstallEventHandler() { | ||
| ClassLoader contextClassLoader = currentThread().getContextClassLoader(); | ||
| BIZ_CLASS_LOADER_TO_RELEASER.putIfAbsent(contextClassLoader, new ConcurrentHashMap<>()); | ||
| // 注册资源释放器 | ||
| BIZ_CLASS_LOADER_TO_RELEASER.get(contextClassLoader).putIfAbsent(Log4jResourceReleaser.class.getName(), new Log4jResourceReleaser()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve constructor robustness and extensibility
The constructor has several potential issues:
- Assumes the current thread's context ClassLoader is the correct one
- Hardcodes Log4jResourceReleaser registration
- Lacks error handling for ClassLoader access
Consider:
- Accepting ClassLoader as a parameter
- Implementing a plugin system for releaser registration
- Adding proper error handling
- public ResourceReleaseOnUninstallEventHandler() {
- ClassLoader contextClassLoader = currentThread().getContextClassLoader();
- BIZ_CLASS_LOADER_TO_RELEASER.putIfAbsent(contextClassLoader, new ConcurrentHashMap<>());
- // 注册资源释放器
- BIZ_CLASS_LOADER_TO_RELEASER.get(contextClassLoader).putIfAbsent(Log4jResourceReleaser.class.getName(), new Log4jResourceReleaser());
+ public ResourceReleaseOnUninstallEventHandler(ClassLoader targetClassLoader) {
+ if (targetClassLoader == null) {
+ targetClassLoader = currentThread().getContextClassLoader();
+ }
+ try {
+ BIZ_CLASS_LOADER_TO_RELEASER.putIfAbsent(targetClassLoader, new ConcurrentHashMap<>());
+ registerReleasers(targetClassLoader);
+ } catch (Exception e) {
+ LOGGER.error("Failed to initialize ResourceReleaseOnUninstallEventHandler", e);
+ throw new RuntimeException("Failed to initialize resource releaser", e);
+ }
+ }
+
+ protected void registerReleasers(ClassLoader classLoader) {
+ // Register default releasers
+ BIZ_CLASS_LOADER_TO_RELEASER.get(classLoader)
+ .putIfAbsent(Log4jResourceReleaser.class.getName(), new Log4jResourceReleaser());
+ // Additional releasers can be registered here or through a plugin system
}📝 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.
| public ResourceReleaseOnUninstallEventHandler() { | |
| ClassLoader contextClassLoader = currentThread().getContextClassLoader(); | |
| BIZ_CLASS_LOADER_TO_RELEASER.putIfAbsent(contextClassLoader, new ConcurrentHashMap<>()); | |
| // 注册资源释放器 | |
| BIZ_CLASS_LOADER_TO_RELEASER.get(contextClassLoader).putIfAbsent(Log4jResourceReleaser.class.getName(), new Log4jResourceReleaser()); | |
| } | |
| public ResourceReleaseOnUninstallEventHandler(ClassLoader targetClassLoader) { | |
| if (targetClassLoader == null) { | |
| targetClassLoader = currentThread().getContextClassLoader(); | |
| } | |
| try { | |
| BIZ_CLASS_LOADER_TO_RELEASER.putIfAbsent(targetClassLoader, new ConcurrentHashMap<>()); | |
| registerReleasers(targetClassLoader); | |
| } catch (Exception e) { | |
| LOGGER.error("Failed to initialize ResourceReleaseOnUninstallEventHandler", e); | |
| throw new RuntimeException("Failed to initialize resource releaser", e); | |
| } | |
| } | |
| protected void registerReleasers(ClassLoader classLoader) { | |
| // Register default releasers | |
| BIZ_CLASS_LOADER_TO_RELEASER.get(classLoader) | |
| .putIfAbsent(Log4jResourceReleaser.class.getName(), new Log4jResourceReleaser()); | |
| // Additional releasers can be registered here or through a plugin system | |
| } |
| @Override | ||
| public void handleEvent(BeforeBizRecycleEvent event) { | ||
| ClassLoader bizClassLoader = event.getSource().getBizClassLoader(); | ||
| LOGGER.info( | ||
| "[ResourceReleaseOnUninstallEventHandler] Module name: {} , BizClassLoader: {} .", | ||
| event.getSource().getBizName(), bizClassLoader); | ||
|
|
||
| ConcurrentHashMap<String, ResourceReleaser> releasers = BIZ_CLASS_LOADER_TO_RELEASER.get(bizClassLoader); | ||
| if (releasers == null) { | ||
| LOGGER.info( | ||
| "[ResourceReleaseOnUninstallEventHandler] No releasers found for module: {} , just return. ", | ||
| event.getSource().getBizName()); | ||
| return; | ||
| } | ||
|
|
||
| LOGGER.info( | ||
| "[ResourceReleaseOnUninstallEventHandler] {} releasers found for module: {} . ", | ||
| releasers.size(), event.getSource().getBizName()); | ||
| for (ResourceReleaser releaser : releasers.values()) { | ||
| releaser.doRelease(event.getSource()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling and map cleanup in event handler
The event handling logic needs improvement in several areas:
- Missing error handling during releaser execution
- No cleanup of the ClassLoader entry from the static map
- No handling of partial failures during release
@Override
public void handleEvent(BeforeBizRecycleEvent event) {
ClassLoader bizClassLoader = event.getSource().getBizClassLoader();
LOGGER.info(
"[ResourceReleaseOnUninstallEventHandler] Module name: {} , BizClassLoader: {} .",
event.getSource().getBizName(), bizClassLoader);
ConcurrentHashMap<String, ResourceReleaser> releasers = BIZ_CLASS_LOADER_TO_RELEASER.get(bizClassLoader);
if (releasers == null) {
LOGGER.info(
"[ResourceReleaseOnUninstallEventHandler] No releasers found for module: {} , just return. ",
event.getSource().getBizName());
return;
}
LOGGER.info(
"[ResourceReleaseOnUninstallEventHandler] {} releasers found for module: {} . ",
releasers.size(), event.getSource().getBizName());
+ boolean hasErrors = false;
for (ResourceReleaser releaser : releasers.values()) {
+ try {
releaser.doRelease(event.getSource());
+ } catch (Exception e) {
+ hasErrors = true;
+ LOGGER.error("Failed to release resources for {} using {}",
+ event.getSource().getBizName(), releaser.getClass().getName(), e);
+ }
}
+
+ // Clean up the map entry
+ BIZ_CLASS_LOADER_TO_RELEASER.remove(bizClassLoader);
+
+ if (hasErrors) {
+ throw new RuntimeException("Some resources failed to release properly. Check logs for details.");
+ }
}📝 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.
| @Override | |
| public void handleEvent(BeforeBizRecycleEvent event) { | |
| ClassLoader bizClassLoader = event.getSource().getBizClassLoader(); | |
| LOGGER.info( | |
| "[ResourceReleaseOnUninstallEventHandler] Module name: {} , BizClassLoader: {} .", | |
| event.getSource().getBizName(), bizClassLoader); | |
| ConcurrentHashMap<String, ResourceReleaser> releasers = BIZ_CLASS_LOADER_TO_RELEASER.get(bizClassLoader); | |
| if (releasers == null) { | |
| LOGGER.info( | |
| "[ResourceReleaseOnUninstallEventHandler] No releasers found for module: {} , just return. ", | |
| event.getSource().getBizName()); | |
| return; | |
| } | |
| LOGGER.info( | |
| "[ResourceReleaseOnUninstallEventHandler] {} releasers found for module: {} . ", | |
| releasers.size(), event.getSource().getBizName()); | |
| for (ResourceReleaser releaser : releasers.values()) { | |
| releaser.doRelease(event.getSource()); | |
| } | |
| } | |
| @Override | |
| public void handleEvent(BeforeBizRecycleEvent event) { | |
| ClassLoader bizClassLoader = event.getSource().getBizClassLoader(); | |
| LOGGER.info( | |
| "[ResourceReleaseOnUninstallEventHandler] Module name: {} , BizClassLoader: {} .", | |
| event.getSource().getBizName(), bizClassLoader); | |
| ConcurrentHashMap<String, ResourceReleaser> releasers = BIZ_CLASS_LOADER_TO_RELEASER.get(bizClassLoader); | |
| if (releasers == null) { | |
| LOGGER.info( | |
| "[ResourceReleaseOnUninstallEventHandler] No releasers found for module: {} , just return. ", | |
| event.getSource().getBizName()); | |
| return; | |
| } | |
| LOGGER.info( | |
| "[ResourceReleaseOnUninstallEventHandler] {} releasers found for module: {} . ", | |
| releasers.size(), event.getSource().getBizName()); | |
| boolean hasErrors = false; | |
| for (ResourceReleaser releaser : releasers.values()) { | |
| try { | |
| releaser.doRelease(event.getSource()); | |
| } catch (Exception e) { | |
| hasErrors = true; | |
| LOGGER.error("Failed to release resources for {} using {}", | |
| event.getSource().getBizName(), releaser.getClass().getName(), e); | |
| } | |
| } | |
| // Clean up the map entry | |
| BIZ_CLASS_LOADER_TO_RELEASER.remove(bizClassLoader); | |
| if (hasErrors) { | |
| throw new RuntimeException("Some resources failed to release properly. Check logs for details."); | |
| } | |
| } |
| static final ConcurrentHashMap<ClassLoader, ConcurrentHashMap<String, ResourceReleaser>> BIZ_CLASS_LOADER_TO_RELEASER = | ||
| new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
💡 Codebase verification
Memory leak concern is valid - map entries are not removed after use
After reviewing the implementation, the concern about memory leaks is confirmed. The handleEvent method retrieves and uses the releasers from BIZ_CLASS_LOADER_TO_RELEASER but never removes the entries after module uninstallation. This will cause the map to retain references to ClassLoaders of uninstalled modules.
Key issues:
- No cleanup of
BIZ_CLASS_LOADER_TO_RELEASERentries afterdoReleaseis called - ClassLoader references are held indefinitely in the static map
- The test case verifies resource release but doesn't verify map cleanup
Recommendation: Add BIZ_CLASS_LOADER_TO_RELEASER.remove(bizClassLoader) after the release loop in handleEvent method.
🔗 Analysis chain
Consider adding cleanup mechanism for static map
The static BIZ_CLASS_LOADER_TO_RELEASER map could potentially lead to memory leaks if ClassLoader entries aren't removed after module uninstallation. Consider:
- Adding a cleanup mechanism in the event handler
- Using WeakHashMap or maintaining explicit lifecycle management
- Adding documentation about thread-safety guarantees
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's any cleanup mechanism in other handlers
rg -l "BIZ_CLASS_LOADER_TO_RELEASER\.remove"
Length of output: 46
Script:
#!/bin/bash
# Let's check the implementation of the event handler to see how the map is managed
ast-grep --pattern 'class ResourceReleaseOnUninstallEventHandler {
$$$
public void handle($_) {
$$$
}
$$$
}'
# Also check for any clear/remove operations on the map
rg -A 5 "BIZ_CLASS_LOADER_TO_RELEASER"
# Check if there are any weak references used
rg "WeakReference|WeakHashMap"
Length of output: 3759
Script:
#!/bin/bash
# Let's check the complete event handler implementation to see if cleanup happens during uninstallation
ast-grep --pattern 'public void handleEvent(BeforeBizRecycleEvent event) {
$$$
}'
# Also check for any other methods in this class that might handle cleanup
ast-grep --pattern 'class ResourceReleaseOnUninstallEventHandler {
$$$
private void $_($_) {
$$$
}
$$$
}'
# Check if there are any other classes that might access and clean this map
rg -l "ResourceReleaseOnUninstallEventHandler"
Length of output: 620
Script:
#!/bin/bash
# Let's check the test file to understand the expected behavior
cat koupleless-base-plugin/src/test/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResouceReleaseOnUninstallEventHandlerTest.java
# Check the activator to see how this handler is used
rg -A 5 "ResourceReleaseOnUninstallEventHandler" koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/ServerlessRuntimeActivator.java
Length of output: 2955
Script:
#!/bin/bash
# Let's check the complete implementation of handleEvent method to understand the cleanup logic
cat koupleless-base-plugin/src/main/java/com/alipay/sofa/koupleless/plugin/manager/handler/ResourceReleaseOnUninstallEventHandler.java
# Also check the ResourceReleaser interface to understand the cleanup contract
rg -l "interface.*ResourceReleaser"
Length of output: 2767
|
|
||
| generateTestLoggerContextFactory(); | ||
|
|
||
| assertEquals(true, testLoggerContext.isStarted() || testLoggerContext.isStarting()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test assertions and add error scenarios
The current test could be enhanced in several ways:
- Use more expressive assertions (e.g.,
assertThat()with descriptive matchers) - Add test cases for error scenarios
- Clean up system properties after the test
- assertEquals(true, testLoggerContext.isStarted() || testLoggerContext.isStarting());
+ assertThat("Logger context should be active before event handling",
+ testLoggerContext.isStarted() || testLoggerContext.isStarting(),
+ is(true));
- assertEquals(true, testLoggerContext.isStopped() || testLoggerContext.isStopping());
+ assertThat("Logger context should be inactive after event handling",
+ testLoggerContext.isStopped() || testLoggerContext.isStopping(),
+ is(true));Also, consider adding these test cases:
@Test
public void testHandleReleaseWithNullEvent() {
assertThrows(IllegalArgumentException.class, () ->
new ResourceReleaseOnUninstallEventHandler().handleEvent(null));
}
@Test
public void testHandleReleaseWithNullBizModel() {
BeforeBizRecycleEvent event = new BeforeBizRecycleEvent(null);
assertThrows(IllegalArgumentException.class, () ->
new ResourceReleaseOnUninstallEventHandler().handleEvent(event));
}Also applies to: 70-70
| public void tearDown() { | ||
| currentThread().setContextClassLoader(originalClassLoader); | ||
| } |
There was a problem hiding this comment.
Clean up system properties in tearDown
The test sets system properties but doesn't clean them up, which could affect other tests.
@After
public void tearDown() {
currentThread().setContextClassLoader(originalClassLoader);
+ System.clearProperty("log4j2.loggerContextFactory");
+ System.clearProperty("log4j.configurationFile");
+ if (testLoggerContext != null) {
+ testLoggerContext.stop();
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation