From ac9af69eee9636ff98c2b60224964e518aebb421 Mon Sep 17 00:00:00 2001 From: Alan Bateman Date: Sun, 1 Jun 2025 06:17:50 +0000 Subject: [PATCH] 8357637: Native resources cached in thread locals not released when FJP common pool threads clears thread locals Reviewed-by: vklang --- .../java/lang/InheritableThreadLocal.java | 6 +-- .../share/classes/java/lang/System.java | 4 -- .../share/classes/java/lang/Thread.java | 35 ++++++++++-- .../share/classes/java/lang/ThreadLocal.java | 36 +++++-------- .../jdk/internal/access/JavaLangAccess.java | 6 --- .../jdk/internal/misc/CarrierThreadLocal.java | 6 +-- .../internal/misc/TerminatingThreadLocal.java | 23 ++++---- .../classes/sun/nio/ch/IOVecWrapper.java | 3 +- .../TestTerminatingThreadLocal.java | 53 ++++++++++++++++++- 9 files changed, 112 insertions(+), 60 deletions(-) diff --git a/src/java.base/share/classes/java/lang/InheritableThreadLocal.java b/src/java.base/share/classes/java/lang/InheritableThreadLocal.java index 10543314ead..ebc95e58418 100644 --- a/src/java.base/share/classes/java/lang/InheritableThreadLocal.java +++ b/src/java.base/share/classes/java/lang/InheritableThreadLocal.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -80,7 +80,7 @@ public class InheritableThreadLocal extends ThreadLocal { */ @Override ThreadLocalMap getMap(Thread t) { - return t.inheritableThreadLocals; + return t.inheritableThreadLocals(); } /** @@ -91,6 +91,6 @@ public class InheritableThreadLocal extends ThreadLocal { */ @Override void createMap(Thread t, T firstValue) { - t.inheritableThreadLocals = new ThreadLocalMap(this, firstValue); + t.setInheritableThreadLocals(new ThreadLocalMap(this, firstValue)); } } diff --git a/src/java.base/share/classes/java/lang/System.java b/src/java.base/share/classes/java/lang/System.java index 882b5f6945f..bcdbc39e665 100644 --- a/src/java.base/share/classes/java/lang/System.java +++ b/src/java.base/share/classes/java/lang/System.java @@ -2254,10 +2254,6 @@ public final class System { ((ThreadLocal)local).removeCarrierThreadLocal(); } - public boolean isCarrierThreadLocalPresent(CarrierThreadLocal local) { - return ((ThreadLocal)local).isCarrierThreadLocalPresent(); - } - public Object[] scopedValueCache() { return Thread.scopedValueCache(); } diff --git a/src/java.base/share/classes/java/lang/Thread.java b/src/java.base/share/classes/java/lang/Thread.java index 9daae01676f..2fc20a94f1d 100644 --- a/src/java.base/share/classes/java/lang/Thread.java +++ b/src/java.base/share/classes/java/lang/Thread.java @@ -235,7 +235,7 @@ public class Thread implements Runnable { private volatile ClassLoader contextClassLoader; // Additional fields for platform threads. - // All fields, except task, are accessed directly by the VM. + // All fields, except task and terminatingThreadLocals, are accessed directly by the VM. private static class FieldHolder { final ThreadGroup group; final Runnable task; @@ -244,6 +244,9 @@ public class Thread implements Runnable { volatile boolean daemon; volatile int threadStatus; + // This map is maintained by the ThreadLocal class + ThreadLocal.ThreadLocalMap terminatingThreadLocals; + FieldHolder(ThreadGroup group, Runnable task, long stackSize, @@ -259,17 +262,41 @@ public class Thread implements Runnable { } private final FieldHolder holder; + ThreadLocal.ThreadLocalMap terminatingThreadLocals() { + return holder.terminatingThreadLocals; + } + + void setTerminatingThreadLocals(ThreadLocal.ThreadLocalMap map) { + holder.terminatingThreadLocals = map; + } + /* * ThreadLocal values pertaining to this thread. This map is maintained * by the ThreadLocal class. */ - ThreadLocal.ThreadLocalMap threadLocals; + private ThreadLocal.ThreadLocalMap threadLocals; + + ThreadLocal.ThreadLocalMap threadLocals() { + return threadLocals; + } + + void setThreadLocals(ThreadLocal.ThreadLocalMap map) { + threadLocals = map; + } /* * InheritableThreadLocal values pertaining to this thread. This map is * maintained by the InheritableThreadLocal class. */ - ThreadLocal.ThreadLocalMap inheritableThreadLocals; + private ThreadLocal.ThreadLocalMap inheritableThreadLocals; + + ThreadLocal.ThreadLocalMap inheritableThreadLocals() { + return inheritableThreadLocals; + } + + void setInheritableThreadLocals(ThreadLocal.ThreadLocalMap map) { + inheritableThreadLocals = map; + } /* * Scoped value bindings are maintained by the ScopedValue class. @@ -1492,7 +1519,7 @@ public class Thread implements Runnable { } try { - if (threadLocals != null && TerminatingThreadLocal.REGISTRY.isPresent()) { + if (terminatingThreadLocals() != null) { TerminatingThreadLocal.threadTerminated(); } } finally { diff --git a/src/java.base/share/classes/java/lang/ThreadLocal.java b/src/java.base/share/classes/java/lang/ThreadLocal.java index fae322d2b73..2fb427d8611 100644 --- a/src/java.base/share/classes/java/lang/ThreadLocal.java +++ b/src/java.base/share/classes/java/lang/ThreadLocal.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1997, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1997, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -193,27 +193,6 @@ public class ThreadLocal { return setInitialValue(t); } - /** - * Returns {@code true} if there is a value in the current carrier thread's copy of - * this thread-local variable, even if that values is {@code null}. - * - * @return {@code true} if current carrier thread has associated value in this - * thread-local variable; {@code false} if not - */ - boolean isCarrierThreadLocalPresent() { - assert this instanceof CarrierThreadLocal; - return isPresent(Thread.currentCarrierThread()); - } - - private boolean isPresent(Thread t) { - ThreadLocalMap map = getMap(t); - if (map != null) { - return map.getEntry(this) != null; - } else { - return false; - } - } - /** * Variant of set() to establish initialValue. Used instead * of set() in case user has overridden the set() method. @@ -302,7 +281,11 @@ public class ThreadLocal { * @return the map */ ThreadLocalMap getMap(Thread t) { - return t.threadLocals; + if (this instanceof TerminatingThreadLocal) { + return t.terminatingThreadLocals(); + } else { + return t.threadLocals(); + } } /** @@ -313,7 +296,12 @@ public class ThreadLocal { * @param firstValue value for the initial entry of the map */ void createMap(Thread t, T firstValue) { - t.threadLocals = new ThreadLocalMap(this, firstValue); + var map = new ThreadLocalMap(this, firstValue); + if (this instanceof TerminatingThreadLocal) { + t.setTerminatingThreadLocals(map); + } else { + t.setThreadLocals(map); + } } /** diff --git a/src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java b/src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java index 1f60f85464b..5109e7b2a5b 100644 --- a/src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java +++ b/src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java @@ -546,12 +546,6 @@ public interface JavaLangAccess { */ void removeCarrierThreadLocal(CarrierThreadLocal local); - /** - * Returns {@code true} if there is a value in the current carrier thread's copy of - * thread-local, even if that values is {@code null}. - */ - boolean isCarrierThreadLocalPresent(CarrierThreadLocal local); - /** * Returns the current thread's scoped values cache */ diff --git a/src/java.base/share/classes/jdk/internal/misc/CarrierThreadLocal.java b/src/java.base/share/classes/jdk/internal/misc/CarrierThreadLocal.java index b14ecaffa2e..9396521cad5 100644 --- a/src/java.base/share/classes/jdk/internal/misc/CarrierThreadLocal.java +++ b/src/java.base/share/classes/jdk/internal/misc/CarrierThreadLocal.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2022, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -49,9 +49,5 @@ public class CarrierThreadLocal extends ThreadLocal { JLA.removeCarrierThreadLocal(this); } - public boolean isPresent() { - return JLA.isCarrierThreadLocalPresent(this); - } - private static final JavaLangAccess JLA = SharedSecrets.getJavaLangAccess(); } diff --git a/src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java b/src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java index eeb1a77e226..beee5b78d1a 100644 --- a/src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java +++ b/src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -29,10 +29,9 @@ import java.util.Collections; import java.util.IdentityHashMap; /** - * A per-carrier-thread-local variable that is notified when a thread terminates and - * it has been initialized in the terminating carrier thread or a virtual thread - * that had the terminating carrier thread as its carrier thread (even if it was - * initialized with a null value). + * A platform thread-local variable that is notified when a platform thread terminates, + * and it has been initialized in the terminating thread (or a mounted virtual thread in + * the case of a carrier thread), and even if it was initialized with a null value. */ public class TerminatingThreadLocal extends CarrierThreadLocal { @@ -44,8 +43,8 @@ public class TerminatingThreadLocal extends CarrierThreadLocal { @Override public void remove() { - super.remove(); unregister(this); + super.remove(); } /** @@ -80,7 +79,9 @@ public class TerminatingThreadLocal extends CarrierThreadLocal { * @param tl the ThreadLocal to register */ public static void register(TerminatingThreadLocal tl) { - REGISTRY.get().add(tl); + if (tl != REGISTRY) { + REGISTRY.get().add(tl); + } } /** @@ -93,11 +94,11 @@ public class TerminatingThreadLocal extends CarrierThreadLocal { } /** - * a per-carrier-thread registry of TerminatingThreadLocal(s) that have been registered - * but later not unregistered in a particular carrier-thread. + * A per-platform-thread registry of TerminatingThreadLocal(s). The registry is + * itself a TerminatingThreadLocal to keep it reachable until the thread terminates. */ - public static final CarrierThreadLocal>> REGISTRY = - new CarrierThreadLocal<>() { + public static final TerminatingThreadLocal>> REGISTRY = + new TerminatingThreadLocal<>() { @Override protected Collection> initialValue() { return Collections.newSetFromMap(new IdentityHashMap<>(4)); diff --git a/src/java.base/share/classes/sun/nio/ch/IOVecWrapper.java b/src/java.base/share/classes/sun/nio/ch/IOVecWrapper.java index 1c78e45a2d8..6d551462ce8 100644 --- a/src/java.base/share/classes/sun/nio/ch/IOVecWrapper.java +++ b/src/java.base/share/classes/sun/nio/ch/IOVecWrapper.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2000, 2024, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2000, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -79,6 +79,7 @@ class IOVecWrapper { protected void threadTerminated(IOVecWrapper[] cache) { IOVecWrapper wrapper = cache[0]; if (wrapper != null) { + cache[0] = null; wrapper.vecArray.free(); } } diff --git a/test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java b/test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java index 00bddec6b56..cf46c5b1d22 100644 --- a/test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java +++ b/test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -24,6 +24,7 @@ import jdk.internal.misc.TerminatingThreadLocal; import java.lang.reflect.Constructor; +import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.List; @@ -31,6 +32,7 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.Executor; import java.util.concurrent.Executors; import java.util.concurrent.ThreadFactory; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Stream; @@ -41,7 +43,7 @@ import static org.testng.Assert.*; /* * @test - * @bug 8202788 8291897 + * @bug 8202788 8291897 8357637 * @summary TerminatingThreadLocal unit test * @modules java.base/java.lang:+open java.base/jdk.internal.misc * @requires vm.continuations @@ -154,6 +156,53 @@ public class TestTerminatingThreadLocal { assertEquals(terminatedValues, expectedTerminatedValues); } + /** + * Test TerminatingThreadLocal when thread locals are "cleared" by null'ing the + * threadLocal field of the current Thread. + */ + @Test + public void testClearingThreadLocals() throws Throwable { + var terminatedValues = new CopyOnWriteArrayList(); + + var tl = new ThreadLocal(); + var ttl = new TerminatingThreadLocal() { + @Override + protected void threadTerminated(String value) { + terminatedValues.add(value); + } + }; + var throwableRef = new AtomicReference(); + + String tlValue = "abc"; + String ttlValue = "xyz"; + + Thread thread = Thread.ofPlatform().start(() -> { + try { + tl.set(tlValue); + ttl.set(ttlValue); + + assertEquals(tl.get(), tlValue); + assertEquals(ttl.get(), ttlValue); + + // set Thread.threadLocals to null + Field f = Thread.class.getDeclaredField("threadLocals"); + f.setAccessible(true); + f.set(Thread.currentThread(), null); + + assertNull(tl.get()); + assertEquals(ttl.get(), ttlValue); + } catch (Throwable t) { + throwableRef.set(t); + } + }); + thread.join(); + if (throwableRef.get() instanceof Throwable t) { + throw t; + } + + assertEquals(terminatedValues, List.of(ttlValue)); + } + /** * Returns a builder to create virtual threads that use the given scheduler. */