davidzollo commented on PR #10431: URL: https://github.com/apache/seatunnel/pull/10431#issuecomment-3830426400
Thanks for the contribution! This feature is very valuable for flexible deployment scenarios where client and server paths differ. However, after a detailed review, I found a **critical bug** in the ClassLoader cache management that will cause memory leaks, and a robustness issue in the path resolver. ### 1. Critical: Key Mismatch in ClassLoader Cache (Memory Leak) There is an inconsistency in how the cache `key` is calculated between getting and releasing the classloader. * In `getClassLoader` (Line 71), the key is calculated using the primitive `jars` (which contain the **logical path** with `$SEATUNNEL_HOME`). * In `releaseClassLoader` (Lines 112-123), you resolve the paths to **absolute paths** *before* calculating the key. ```java // DefaultClassLoaderService.java // In getClassLoader: String key = covertJarsToKey(jars); // Key is based on "file:$SEATUNNEL_HOME/..." // In releaseClassLoader: Collection<URL> resolvedJars = new ArrayList<>(jars); PathResolver.resolvePathEnv(resolvedJars); // ... String key = covertJarsToKey(resolvedJars); // Key is based on "file:/opt/seatunnel/..." ``` **Consequence:** The keys will never match. `releaseClassLoader` will fail to look up the entry in `classLoaderCache`. The reference count will never decrement, and the ClassLoader will **never be released**, leading to a Permanent Generation memory leak for long-running processes. **Fix Suggestion:** In `releaseClassLoader`, do **not** use `resolvedJars` to generate the key. Use the original `jars` collection to ensure the key matches the one put into the map during `getClassLoader`. ### 2. NPE Risk in `PathResolver` In `PathResolver.java`, `Common.getSeaTunnelHome()` can return `null` if the environment is not set up correctly. ```java // PathResolver.java String home = Common.getSeaTunnelHome(); // ... String normalizedHome = new File(home).getAbsolutePath(); // This throws NPE if home is null ``` **Fix Suggestion:** Please add a null check for `home`. If `SEATUNNEL_HOME` is not found, it should either skip the replacement logic safely or throw a descriptive exception rather than a raw NullPointerException. ### 3. Testing I highly recommend adding a test case (perhaps in `PathResolverTest` or an IT) that explicitly sets `SEATUNNEL_HOME` to `null` to verify robustness. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
