From 318eb3a2483d5cc2be729854b21f69694edcde53 Mon Sep 17 00:00:00 2001 From: Jinwoo Hwang Date: Fri, 14 Nov 2025 08:58:55 -0500 Subject: [PATCH 1/2] Replace reflection-based UnsafeThreadLocal with WeakHashMap implementation - Removed reflection access to ThreadLocal/ThreadLocalMap internals - Implemented cross-thread value lookup using synchronized WeakHashMap - Removed requirement for --add-opens=java.base/java.lang=ALL-UNNAMED - WeakHashMap ensures terminated threads can be garbage collected - Maintains same API and functionality for deadlock detection - All existing tests pass without JVM flag changes This eliminates the fragile reflection-based approach that required special JVM flags and was vulnerable to Java module system changes. The new implementation is safer, more maintainable, and works across all Java versions without requiring internal access. --- .../internal/deadlock/UnsafeThreadLocal.java | 100 +++++++++--------- .../cli/commands/MemberJvmOptions.java | 6 -- 2 files changed, 48 insertions(+), 58 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java index 17872c29cb65..afabb84722d2 100644 --- a/geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java +++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/deadlock/UnsafeThreadLocal.java @@ -14,72 +14,68 @@ */ package org.apache.geode.distributed.internal.deadlock; -import java.lang.reflect.Field; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; +import java.util.Map; +import java.util.WeakHashMap; /** - * Most of this thread local is safe to use, except for the getValue(Thread) method. That is not - * guaranteed to be correct. But for our deadlock detection tool I think it's good enough, and this - * class provides a very low overhead way for us to record what thread holds a particular resource. + * A ThreadLocal implementation that allows reading values from arbitrary threads, useful for + * deadlock detection. This implementation uses a WeakHashMap to track values per thread without + * requiring reflection or JVM internal access. * + *

+ * Unlike standard ThreadLocal, this class maintains an additional mapping that allows querying the + * value for any thread, not just the current thread. This is useful for deadlock detection where + * we need to inspect what resources other threads are holding. + *

* + *

+ * The implementation uses WeakHashMap with Thread keys to ensure threads can be garbage collected + * when they terminate, preventing memory leaks. + *

*/ public class UnsafeThreadLocal extends ThreadLocal { /** - * Dangerous method. Uses reflection to extract the thread local for a given thread. - * - * Unlike get(), this method does not set the initial value if none is found - * + * Maps threads to their values. Uses WeakHashMap so terminated threads can be GC'd. Synchronized + * to ensure thread-safe access. */ - public T get(Thread thread) { - return (T) get(this, thread); - } + private final Map threadValues = + java.util.Collections.synchronizedMap(new WeakHashMap<>()); - private static Object get(ThreadLocal threadLocal, Thread thread) { - try { - Object threadLocalMap = - invokePrivate(threadLocal, "getMap", new Class[] {Thread.class}, new Object[] {thread}); - - if (threadLocalMap != null) { - Object entry = invokePrivate(threadLocalMap, "getEntry", new Class[] {ThreadLocal.class}, - new Object[] {threadLocal}); - if (entry != null) { - return getPrivate(entry, "value"); - } - } - return null; - } catch (Exception e) { - throw new RuntimeException("Unable to extract thread local", e); + /** + * Sets the value for the current thread and records it in the cross-thread map. + */ + @Override + public void set(T value) { + super.set(value); + if (value != null) { + threadValues.put(Thread.currentThread(), value); + } else { + threadValues.remove(Thread.currentThread()); } } - private static Object getPrivate(Object object, String fieldName) throws SecurityException, - NoSuchFieldException, IllegalArgumentException, IllegalAccessException { - Field field = object.getClass().getDeclaredField(fieldName); - field.setAccessible(true); - return field.get(object); + /** + * Removes the value for the current thread from both the ThreadLocal and the cross-thread map. + */ + @Override + public void remove() { + super.remove(); + threadValues.remove(Thread.currentThread()); } - private static Object invokePrivate(Object object, String methodName, Class[] argTypes, - Object[] args) throws SecurityException, NoSuchMethodException, IllegalArgumentException, - IllegalAccessException, InvocationTargetException { - - Method method = null; - Class clazz = object.getClass(); - while (method == null) { - try { - method = clazz.getDeclaredMethod(methodName, argTypes); - } catch (NoSuchMethodException e) { - clazz = clazz.getSuperclass(); - if (clazz == null) { - throw e; - } - } - } - method.setAccessible(true); - Object result = method.invoke(object, args); - return result; + /** + * Gets the value for an arbitrary thread, useful for deadlock detection. + * + *

+ * Unlike get(), this method does not set the initial value if none is found. Returns null if the + * specified thread has no value set. + *

+ * + * @param thread the thread whose value to retrieve + * @return the value for the specified thread, or null if none exists + */ + public T get(Thread thread) { + return threadValues.get(thread); } } diff --git a/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java b/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java index dbfc1ee40043..a7ef3878ffcd 100644 --- a/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java +++ b/geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/MemberJvmOptions.java @@ -26,7 +26,6 @@ import java.util.Collections; import java.util.List; -import org.apache.geode.distributed.internal.deadlock.UnsafeThreadLocal; import org.apache.geode.internal.offheap.AddressableMemoryManager; import org.apache.geode.internal.stats50.VMStats50; import org.apache.geode.unsafe.internal.com.sun.jmx.remote.security.MBeanServerAccessController; @@ -44,10 +43,6 @@ public class MemberJvmOptions { */ private static final String COM_SUN_JMX_REMOTE_SECURITY_EXPORT = "--add-exports=java.management/com.sun.jmx.remote.security=ALL-UNNAMED"; - /** - * open needed by {@link UnsafeThreadLocal} - */ - private static final String JAVA_LANG_OPEN = "--add-opens=java.base/java.lang=ALL-UNNAMED"; /** * open needed by {@link AddressableMemoryManager} */ @@ -62,7 +57,6 @@ public class MemberJvmOptions { COM_SUN_JMX_REMOTE_SECURITY_EXPORT, SUN_NIO_CH_EXPORT, COM_SUN_MANAGEMENT_INTERNAL_OPEN, - JAVA_LANG_OPEN, JAVA_NIO_OPEN); public static List getMemberJvmOptions() { From cd9233efcced42823a170e13502e696bce436506 Mon Sep 17 00:00:00 2001 From: Jinwoo Hwang Date: Fri, 14 Nov 2025 09:25:31 -0500 Subject: [PATCH 2/2] Remove --add-opens=java.base/java.lang from test configuration - Removed unnecessary JVM flag from geode-test.gradle line 185 - Flag no longer needed after UnsafeThreadLocal refactoring - Tests now run with same security constraints as production - All UnsafeThreadLocal and deadlock tests pass without the flag - Validates that refactoring truly eliminated reflection dependency --- build-tools/scripts/src/main/groovy/geode-test.gradle | 1 - 1 file changed, 1 deletion(-) diff --git a/build-tools/scripts/src/main/groovy/geode-test.gradle b/build-tools/scripts/src/main/groovy/geode-test.gradle index 93488986e512..602f0b731651 100644 --- a/build-tools/scripts/src/main/groovy/geode-test.gradle +++ b/build-tools/scripts/src/main/groovy/geode-test.gradle @@ -182,7 +182,6 @@ gradle.taskGraph.whenReady({ graph -> if (project.hasProperty('testJVMVer') && testJVMVer.toInteger() >= 9) { jvmArgs += [ "--add-opens=java.base/java.io=ALL-UNNAMED", - "--add-opens=java.base/java.lang=ALL-UNNAMED", "--add-opens=java.base/java.lang.annotation=ALL-UNNAMED", "--add-opens=java.base/java.lang.module=ALL-UNNAMED", "--add-opens=java.base/java.lang.ref=ALL-UNNAMED",