Fix StandardSofaRuntimeManager mem leak #1373
Conversation
Signed-off-by: JermaineHua <[email protected]>
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant ExternalCaller
participant StandardSofaRuntimeManager
participant ClientFactoryInternal
participant ApplicationContext
ExternalCaller->>StandardSofaRuntimeManager: shutDownExternally()
StandardSofaRuntimeManager->>ClientFactoryInternal: destroy()
ClientFactoryInternal-->>StandardSofaRuntimeManager: (clients cleared)
StandardSofaRuntimeManager->>ApplicationContext: close()
StandardSofaRuntimeManager->>ApplicationContext: set rootApplicationContext = null
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 comments (1)
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/StandardSofaRuntimeManager.java (1)
113-126: 💡 Verification agent🧩 Analysis chain
Consider aligning shutdown behavior between methods.
The
shutDownExternally()method now callsdestroy()onclientFactoryInternal, but the regularshutdown()method doesn't. This inconsistency could lead to resource leaks depending on which shutdown path is taken.Consider:
- Adding
clientFactoryInternal.destroy()to theshutdown()method before callingclear().- Calling
clear()inshutDownExternally()to ensure all references are properly nullified.Run the following script to check how these shutdown methods are used in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify usage patterns of shutdown methods to understand the impact # Test 1: Find all calls to shutdown() method echo "=== Calls to shutdown() method ===" rg -A 3 "\.shutdown\(\)" --type java # Test 2: Find all calls to shutDownExternally() method echo -e "\n=== Calls to shutDownExternally() method ===" rg -A 3 "\.shutDownExternally\(\)" --type java # Test 3: Check if ClientFactoryInternal has other cleanup requirements echo -e "\n=== ClientFactoryInternal interface definition ===" fd -e java -x grep -l "interface ClientFactoryInternal" {} \; | xargs catLength of output: 13179
🏁 Script executed:
#!/bin/bash # Extract clear() implementation to verify which fields are reset rg -A 10 "void clear" --type java sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/StandardSofaRuntimeManager.javaLength of output: 519
Align shutdown behaviors in StandardSofaRuntimeManager
To prevent resource leaks and ensure complete cleanup, both
shutdown()andshutDownExternally()should:
- Invoke
clientFactoryInternal.destroy()before dropping the reference.- Call
clear()to null out all internal fields and clear shutdown hooks.Currently:
shutdown()callsclear()(which nullsclientFactoryInternal) but never invokesdestroy().shutDownExternally()invokesdestroy()but never callsclear().Suggested changes:
--- a/.../StandardSofaRuntimeManager.java @@ public void shutdown() throws ServiceRuntimeException { - if (componentManager != null) { - componentManager.shutdown(); - } - clear(); + if (componentManager != null) { + componentManager.shutdown(); + } + if (clientFactoryInternal != null) { + clientFactoryInternal.destroy(); + } + clear(); @@ public void shutDownExternally() throws ServiceRuntimeException { - clientFactoryInternal.destroy(); - AbstractApplicationContext applicationContext = (AbstractApplicationContext) rootApplicationContext; - if (applicationContext.isActive()) { - applicationContext.close(); - } - rootApplicationContext = null; - appClassLoader = null; + if (clientFactoryInternal != null) { + clientFactoryInternal.destroy(); + } + AbstractApplicationContext applicationContext = (AbstractApplicationContext) rootApplicationContext; + if (applicationContext.isActive()) { + applicationContext.close(); + } + clear();This ensures both shutdown paths first destroy the client factory, then null out all references and clear any registered shutdown hooks.
🧹 Nitpick comments (2)
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/spi/client/ClientFactoryInternal.java (1)
36-36: Add JavaDoc documentation for the newdestroy()method.For consistency with the existing code style and to clarify the method's purpose, please add JavaDoc documentation.
Apply this diff to add documentation:
+ /** + * Destroy the client factory and release all registered clients. + */ void destroy();sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/ClientFactoryImpl.java (1)
32-46: Consider thread safety during shutdown sequence.While
ConcurrentHashMapoperations are thread-safe individually, there might be race conditions ifregisterClient()orgetClient()are called concurrently withdestroy(). Consider documenting the expected shutdown behavior or adding a state check to prevent operations after destruction.Consider implementing a shutdown state to prevent new registrations after destroy:
public class ClientFactoryImpl implements ClientFactoryInternal { private final Map<Class<?>, Object> clients = new ConcurrentHashMap<>(8); + private volatile boolean destroyed = false; @Override public void registerClient(Class<?> clientType, Object clientInstance) { + if (destroyed) { + throw new IllegalStateException("ClientFactory has been destroyed"); + } if (clients.containsKey(clientType)) { return; } clients.put(clientType, clientInstance); } @Override public void destroy() { + destroyed = true; clients.clear(); } @SuppressWarnings("unchecked") @Override public <T> T getClient(Class<T> clazz) { + if (destroyed) { + return null; + } return (T) clients.get(clazz); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/ClientFactoryImpl.java(1 hunks)sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/StandardSofaRuntimeManager.java(1 hunks)sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/spi/client/ClientFactoryInternal.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (21)
- GitHub Check: build (17)
- GitHub Check: sca
🔇 Additional comments (2)
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/StandardSofaRuntimeManager.java (1)
121-121: Good addition for preventing memory leaks!Setting
rootApplicationContextto null after closing it helps ensure proper garbage collection and prevents potential memory leaks.sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/ClientFactoryImpl.java (1)
43-46: LGTM! The implementation correctly clears the client map.The use of
ConcurrentHashMap.clear()ensures thread-safe removal of all entries.
| @@ -112,11 +112,13 @@ public void shutdown() throws ServiceRuntimeException { | |||
| @Override | |||
| public void shutDownExternally() throws ServiceRuntimeException { | |||
| try { | |||
| clientFactoryInternal.destroy(); | |||
There was a problem hiding this comment.
Add null check before calling destroy().
While adding the destroy() call is good for preventing memory leaks, consider adding a null check for defensive programming:
Apply this diff to add null check:
-clientFactoryInternal.destroy();
+if (clientFactoryInternal != null) {
+ clientFactoryInternal.destroy();
+}🤖 Prompt for AI Agents
In
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/StandardSofaRuntimeManager.java
at line 115, the destroy() method is called without checking if
clientFactoryInternal is null, which could lead to a NullPointerException. Add a
null check before calling destroy() to ensure safe execution and prevent
potential runtime errors.
| @Override | ||
| public void destroy() { | ||
| clients.clear(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider resource cleanup for client instances.
If the client instances themselves hold resources (e.g., connections, thread pools, file handles), simply clearing the map might not be sufficient. Consider whether clients need their own cleanup methods to be called before removal.
Run the following script to check if any client implementations have cleanup-related methods:
🏁 Script executed:
#!/bin/bash
# Description: Search for client implementations that might have cleanup methods
# Search for classes that might be registered as clients
ast-grep --pattern 'class $_ {
$$$
close($$$) {
$$$
}
$$$
}'
# Also search for shutdown/destroy/cleanup methods
rg -A 3 "(void\s+(shutdown|destroy|cleanup|close|dispose)\s*\()"Length of output: 10095
#!/bin/bash
# Display the ClientFactoryInternal interface
fd ClientFactoryInternal.java -x sed -n '1,200p' {}
# Display the ClientFactoryImpl implementation and clients field
fd ClientFactoryImpl.java -x sed -n '1,200p' {}Enhance ClientFactoryImpl.destroy() to invoke client cleanup
Since ClientFactoryImpl simply clears the clients map, any client‐specific resources (connections, threads, etc.) won’t be released. Even though we didn’t find a uniform “cleanup” method on those instances, it’s safest to:
- Iterate over
clients.values() - For each value, detect and invoke cleanup if it implements
AutoCloseable, Spring’sDisposableBean, or declares adestroy()/close()method via reflection - Then clear the map
Locations to address:
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/ClientFactoryImpl.java:destroy()
Suggested diff sketch:
public void destroy() {
- clients.clear();
+ for (Object client : clients.values()) {
+ if (client instanceof AutoCloseable) {
+ try { ((AutoCloseable) client).close(); } catch (Exception ignored) {}
+ } else {
+ // reflectively call destroy() or close() if present
+ invokeIfExists(client, "destroy");
+ invokeIfExists(client, "close");
+ }
+ }
+ clients.clear();
}
+private void invokeIfExists(Object obj, String methodName) {
+ try {
+ Method m = obj.getClass().getMethod(methodName);
+ m.invoke(obj);
+ } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException ignored) {}
+}This ensures clients with cleanup hooks are properly closed before removal.
📝 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 destroy() { | |
| clients.clear(); | |
| } | |
| @Override | |
| public void destroy() { | |
| for (Object client : clients.values()) { | |
| if (client instanceof AutoCloseable) { | |
| try { | |
| ((AutoCloseable) client).close(); | |
| } catch (Exception ignored) { | |
| } | |
| } else { | |
| // reflectively call destroy() or close() if present | |
| invokeIfExists(client, "destroy"); | |
| invokeIfExists(client, "close"); | |
| } | |
| } | |
| clients.clear(); | |
| } | |
| private void invokeIfExists(Object obj, String methodName) { | |
| try { | |
| Method m = obj.getClass().getMethod(methodName); | |
| m.invoke(obj); | |
| } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException ignored) { | |
| } | |
| } |
🤖 Prompt for AI Agents
In
sofa-boot-project/sofa-boot-core/runtime-sofa-boot/src/main/java/com/alipay/sofa/runtime/impl/ClientFactoryImpl.java
lines 43 to 46, the destroy method only clears the clients map without invoking
any cleanup methods on the client instances. To properly release resources,
iterate over the values in the clients map, check if each client implements
AutoCloseable, DisposableBean, or has a destroy() or close() method via
reflection, invoke the appropriate cleanup method, and then clear the map. This
ensures client resources are released correctly before removal.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1373 +/- ##
============================================
+ Coverage 82.91% 82.94% +0.02%
- Complexity 2971 2973 +2
============================================
Files 340 340
Lines 9828 9832 +4
Branches 1178 1178
============================================
+ Hits 8149 8155 +6
Misses 1164 1164
+ Partials 515 513 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Bug Fixes