knight6236 commented on PR #10678:
URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5223808331
@SEZ9 @nzw921rx Thank you both for the thorough reviews and for providing
different perspectives. I’d like to respond to all the points together.
---
1. JDK 9+ --add-opens for jdk.internal.loader
You are absolutely right – I missed the additional --add-opens
java.base/jdk.internal.loader=ALL-UNNAMED required for reflecting into
URLClassPath fields. I will update both the documentation and the WARN log
message at DefaultClassLoaderService.java:324 to include the full set of
required JVM flags.
---
2. JarFileFactory cache clearing and the dual‑switch design
The logic that disables the JAR URL cache is protected by an independent
switch, separate from the deep‑clean switch (SEATUNNEL_CLASSLOADER_DEEP_CLEAN).
Both are disabled by default, so the current behaviour remains unchanged for
all existing deployments. Users who want the complete cleanup path must
explicitly enable both switches – this dual‑guard prevents any unintended side
effect.
I also want to share some context on why we did not pursue a more “precise”
approach like manually cleaning JarFileFactory caches via reflection: in our
earlier experiments, that approach caused severe concurrency issues in
production. JarFileFactory is a JVM‑global singleton, and its internal HashMap
is not designed for concurrent modification. When we reflectively cleared or
replaced cache entries while other worker threads were simultaneously loading
classes/resources, we frequently hit ConcurrentModificationException and even
node instability. That experience led us to the current design – disabling the
global cache once, during the single‑threaded service initialisation phase,
which completely avoids runtime concurrency races.
---
3. JDK 8 global useCaches side effect
I fully confirm that on JDK 8, URLConnection.setDefaultUseCaches(false)
flips the default caching policy for all URL protocols – not only jar:. This is
a fundamental limitation of the JDK 8 API; there is no protocol‑scoped
alternative.
This is exactly the same approach taken by Apache Tomcat’s
JreMemoryLeakPreventionListener. Tomcat’s own source code comments state that
the listener disables the JAR URL connection cache by default to prevent file
locking, and explicitly notes: “Note this disables caching for all
URLConnections, regardless of type.” For Java 9+ they use the protocol‑specific
API, but for Java 8 they rely on the global toggle. This demonstrates that our
solution is aligned with a well‑established, production‑proven practice.
Because the performance impact of globally disabling caches can be
significant (as seen in CXF and other projects), I will add clear warnings in
the configuration documentation and startup logs. Users can then decide whether
to enable the feature based on their own trade‑offs between memory‑leak
prevention and performance.
---
4. JDK 9+ protocol‑scoped cache control
For JDK 9+, I will also add support for the official protocol‑specific API:
URLConnection.setDefaultUseCaches("jar", false). This will allow JDK 9+ users
to disable JAR caching without affecting other protocols. Corresponding test
cases will be included to verify the behaviour.
---
Next steps
All the above fixes (JDK 9+ flag update, documentation/logging improvements,
JDK 9+ protocol‑specific cache handling, and additional tests) will be
submitted in a single commit. After that, I would appreciate a fresh review.
Thank you again for the detailed feedback – it has made the PR much stronger.
--
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]
