RFR: 8357637: Native resources cached in thread locals not released when FJP common pool threads clears thread locals

[Viktor Klang]

On Mon, 26 May 2025 17:16:02 GMT, Alan Bateman <alanb at openjdk.org> wrote:

> A ForkJoinPool can be created with worker threads that clear thread locals between tasks, thus avoiding a build up of thread locals left behind by tasks executed in the pool. The common pool does this. Erasing thread locals (by writing null to Thread.threadLocals) grinds with thread locals that keep native memory alive, esp. when there isn't a Cleaner or other means to free the memory.
> 
> For the JDK, this is currently an issue for the NIO direct buffer cache. If a task running on a thread in the common pool does socket or network channel I/O then it can leak when the task terminates. Prior to JDK 24 each buffer in the cache had a cleaner, as if allocated by ByteBuffer.allocateDirect, so it had some chance of being released. The changes in [JDK-8344882](https://bugs.openjdk.org/browse/JDK-8344882) mean there is no longer is cleaner for buffers in the cache.
> 
> The options in the short term are to restore the cleaner, register a clearer for the buffer cache, have the FJP resetThreadLocals special case "terminating thread locals", or move the terminating thread locals to a different TL map. Viktor Klang, Doug Lea, and I discussed this topic recently and agreed the best short term approach is to split the map. As terminating thread locals are for platform threads then the map can be in the Thread.FieldHolder and avoid adding another field to Thread. Medium to long term require further thought in this area, including seeing what might be useful (and safe) to expose.
> 
> Testing 1-5. Performance testing ongoing.

@AlanBateman I think this approach is a good step in the right direction here.

src/java.base/share/classes/java/lang/InheritableThreadLocal.java line 95:

> 93:     void createMap(Thread t, T firstValue) {
> 94:         var map = new ThreadLocalMap(this, firstValue);
> 95:         t.setInheritableThreadLocals(map);

Stylistic suggestion:

Suggestion:

        t.setInheritableThreadLocals(new ThreadLocalMap(this, firstValue));

src/java.base/share/classes/java/lang/Thread.java line 246:

> 244:         volatile boolean daemon;
> 245:         volatile int threadStatus;
> 246:         ThreadLocal.ThreadLocalMap terminatingThreadLocals;

I think it's worth documenting the memory safety aspects of this field.

src/java.base/share/classes/java/lang/ThreadLocal.java line 284:

> 282:      */
> 283:     ThreadLocalMap getMap(Thread t) {
> 284:         if (this instanceof TerminatingThreadLocal<T>) {

Would it make sense to override `getMap` in TerminatingThreadLocal instead?

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 549:

> 547:     void removeCarrierThreadLocal(CarrierThreadLocal<?> local);
> 548: 
> 549:     /**

Copyright update?

src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java line 82:

> 80:      */
> 81:     public static void register(TerminatingThreadLocal<?> tl) {
> 82:         if (tl != REGISTRY) {

Would this happen in any well-behaved application? If not, might be better to throw an exception? 🤔

src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java line 96:

> 94: 
> 95:     /**
> 96:      * A per-terminating-thread registry of TerminatingThreadLocal(s) that have been

Perhaps "A per-platform-thread registry ..."?

src/java.base/share/classes/sun/nio/ch/IOVecWrapper.java line 83:

> 81:             if (wrapper != null) {
> 82:                 wrapper.vecArray.free();
> 83:                 cache[0] = null;

Perhaps null it out before calling free()? (in case free() fails)

test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java line 194:

> 192:                 assertNull(tl.get());
> 193:                 assertEquals(ttl.get(), ttlValue);
> 194:             } catch (Exception ex) {

Perhaps safer with catching Throwable—if there are any Errors thrown.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/25457#issuecomment-2916569305
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2112057817
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109017596
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109025118
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2112062254
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2115588231
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109416867
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109418667
PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2112066646