DanielLeens commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5203602898
Thanks @SEZ9 for the detailed follow-up. I pulled the exact head you reviewed (`89ff5c3f63c2`, no new commits landed after my last approval on this head) and independently re-verified all three technical points against the current `DefaultClassLoaderService.java` and `ClassLoaderServiceTest.java`. **1. JDK 9+ reflection on `ucp`/`loaders`/`lmap`/`jar` needs `--add-opens`** Confirmed, and there is an extra wrinkle worth calling out: the code's own remediation hint at `DefaultClassLoaderService.java:324` only tells the user to add `--add-opens java.base/java.net=ALL-UNNAMED`. That is enough to reflectively read `URLClassLoader.ucp` (declared in `java.net`), but on JDK 9+ `ucp.getClass()` is `jdk.internal.loader.URLClassPath`, so reflecting into its `loaders`/`lmap` fields at lines 304/314 also needs `--add-opens java.base/jdk.internal.loader=ALL-UNNAMED`. Following the current log message alone will not fully unblock deep clean on JDK 9+. That said, I do not think this makes the primary fix unsound: `closeUrlClassLoader()` (lines 268-282) calls the public `URLClassLoader.close()` API first, unconditionally whenever deep clean is enabled, before any reflection runs. `close()` itself already closes the underlying `JarFile`s (releasing the fds this PR targets) on every JDK version, no `--add-opens` required. `clearUrlClassPathCache()`/`closeJarLoader()` only drop the now-stale `Loader`/`JarFile` object references left behind in `URLClassPath`'s internal collections after `close()` — a heap-retention cleanup on top, not the fd leak itself. So on JDK 9+ without `--add-opens`, the fd/JAR-lock leak this PR targets is still fixed; only the extra reference-clearing degrades gracefully (caught, logged at WARN, consistent with the existing "[Phase 1 Compromise]" framing in the code). **2. `disableJarUrlCache()` mutates JVM-global state unconditionally in the constructor** Confirmed — it runs regardless of the `DEEP_CLEAN` flag (`DefaultClassLoaderService.java:63, 67-76`), guarded only by the static `JAR_CACHE_DISABLED` CAS so it executes once per JVM. It is worth being precise in the PR description that "opt-in" describes the physical-close-plus-reflective-cache-clearing path specifically, not literally every side effect of constructing the service. That said, `URLConnection.setDefaultUseCaches(false)` for `jar:` URLs is the same defensive idiom Tomcat's `WebappClassLoaderBase` uses at startup to avoid the JDK's default-on JAR URL connection cache holding file handles open indefinitely — it does not use reflection, is idempotent, and addresses a related-but-different leak vector than the deep-clean toggle. I would treat this as a documentation-precision nit rather than a safety concern. **3. Static `DEEP_CLEAN_ENABLED`/`JAR_CACHE_DISABLED` — could a second instance silently flip behavior for the first?** Structurally yes, since both are JVM-static `AtomicBoolean`s. I checked where `DefaultClassLoaderService` is actually constructed in production: there is exactly one call site, `SeaTunnelServer.java:158`, invoked once from the node's `init()` per node process, so in a real cluster there is never a second instance sharing the JVM. In the test suite, `ClassLoaderServiceTest` is the only place that constructs extra instances with `DEEP_CLEAN=true`, and each of those tests (`testDeepCleanModeEnabled`, `testClassLoaderClosedOnReleaseWithDeepClean`, `testCloseAllClassLoadersOnServiceCloseWithDeepClean`) already wraps `System.setProperty`/`clearProperty` in try/finally, so I do not see an active cross-test pollution bug today. Still agree this is worth a low-cost follow-up — making deep-clean an instance field resolved once in the constructor rather than a static re-read on every construction would remove the shared-mutable-state smell entirely and make the tests independent of system- property ordering (useful if the suite is ever parallelized). On Issue 1 (doc updates for the new system property) — agreed, Low severity and non-blocking; matches what @knight6236 already said upthread about consolidating docs once Phases 1-4 land. Net: I do not see anything here that reopens a merge blocker beyond what is already tracked as Issue 1. The core leak fix (`URLClassLoader.close()`) sits on the public, non-reflective path and is not gated by JDK version; the riskier reflection/global-cache surface stays behind the opt-in flag and fails safe when it cannot fully execute. My conclusion from the last review stands: no source-level blocker from my side on this head. -- 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]
