There's clearly some issues here but I'd like to figure out how to land a fix.

It seems that the weak references to the class values is too weak, and I'm not sure we should be using weak references at this point anyway. If they're weak and a JavaClass goes unreferenced for a couple of GC cycles, it will end up getting collected and a future access will cause it to be recreated. That's the source of all the reinitialized constant warnings and the reason the test/jruby tests fail here.

Even a SoftReference is probably too weak, since it just delays the time until the JavaClass gets collected.

Is that part of the patch necessary for this to work? Is clearing the reference to the JavaSupport class not enough?

Is that part of the patch necessary for this to work?

Yes it's necessary.

Reverting only the JavaSupport/ClassValue part causes all three GH-9092 regression tests in ScriptingContainerClassLeakTest to fail, and restoring it makes that test class pass again. So that part is necessary for the leak fix, not for normal runtime cache behavior.

If you have a cleaner lifecycle-based alternative, I’m happy to switch to that.

My question was really if we need both changes. Clearing the reference to the JavaSupport instance seems like it would be sufficient, and making the Java class references weak leads to other problems quickly.

I can give it a try myself when I'm in the office.

My question was really if we need both changes. Clearing the reference to the JavaSupport instance seems like it would be sufficient, and making the Java class references weak leads to other problems quickly.

I can give it a try myself when I'm in the office.

I've removed the weak reference and instead, explicitly clear it at teardown. It seems that most tests are passing except that test:mri:core:jit which seem flaky - sometimes java21 failed but java25 passed, and another time java21 passed, but java25 failed. Is this a known issue?

That particular test does seem to be flaky on and off so I don't think it's related to your changes. I will take another look at that but I think we are probably okay with this set of changes.

I think we're largely here but I echo @kares concerns about a few of these changes. It feels to me (begging pardon if this is not the case) that there's a smattering of related attempts to fix this issue that all got balled up in one PR, when the simple fix of fully clearing the JavaSupport reference might solve all of them at once (and in a similar way to other tearDown cleanup).

Let's try to finish this discussion and merge for 10.0.5.0, due out this week.

I've asked copilot to write this response to explain the various changes:

The PR actually started with that simpler approach — the first commit used javaSupport = null and weak-valued caches (ClassValue.newWeakInstance) — but the three leaking reference chains all need independent fixes, so the changes accumulated.

Here's why javaSupport = null alone isn't sufficient:

Path 1 — LocalContext strong reference (the LocalContext.java change)

ScriptingContainer → SingleThreadLocalContextProvider → LocalContext → Ruby (strong field)

LocalContext.remove() never cleared runtime. This keeps the entire runtime strongly reachable from the container itself, regardless of what happens to javaSupport. Fixed by nulling runtime in LocalContext.remove().

Path 2 — ThreadService SoftReference chain (the ThreadService.java change)

Thread (GC root) → ThreadLocalMap → SoftReference<ThreadContext> → ThreadContext → Ruby

SoftReferences are only cleared under heap pressure, which may never occur in a well-resourced long-running server. This specifically affects adopted/pool threads that call into JRuby and then go idle — their ThreadLocalMap entry silently anchors the runtime forever. javaSupport = null doesn't touch this path; the explicit trackedRefs clearing during teardown() is what severs it.

Path 3 — JDK ClassValue external root (the JavaSupport.java change)

Class<X> (JDK GC root) → java.lang.ClassValue table → computed value → lambda capture → Ruby

ClassValue.newInstance() delegates to StableClassValue, which wraps java.lang.ClassValue. That stores computed values inside the Class object itself via JDK internals — not inside JavaSupport. So setting javaSupport = null makes our ClassValue instance unreachable from JRuby's side, but the JDK's own table entries (rooted at Class objects) can only be cleared by the GC's weak-key processing. The switch to MapBasedClassValue eliminates this external root: the entire map is owned through javaSupport, so javaSupport = null (after tearDown()) cuts the chain immediately and deterministically.

The MapBasedClassValue + explicit tearDown() approach (rather than weak values) was chosen specifically in response to earlier reviewer feedback: weak values caused live-runtime cache instability and "already initialized constant" warnings on heavily-threaded reuse.

I found some more small leaks - patched.

Now, after 20+ reloads I don't see any more heap growth, other than those caused by soft references, which takes 1000s to be released by the jvm by default.

@jimtng Thanks for iterating on this.

Regarding the justifications for the additional changes, most seem to make general sense.

I believe the extra effort to clear references throughout the system is appropriate at teardown, and anywhere we are doing so we should be skeptical of decreasing the strength of those references. In other words, if we can actively clear those references and that is sufficient to eventually dereference and collect the entire runtime, no change should be necessary in the strength of those references (Weak or SoftReference additions should not be necessary).

Put yet another way, we should only be using Weak or SoftReference in situations where a still-running JRuby instance does not vacate otherwise unreferenced objects OR if it is not possible at teardown to actively clear those references. This PR is about making teardown clean, so we should focus primarily on that aspect.

This line from Copilot is also not correct (or perhaps it is correct but as a general statement about the behavior of GC:

SoftReferences are only cleared under heap pressure,

This really applies to all objects in a GCed system. If there's no pressure to clear the GC, most GCs will not actively move to vacate the heap. You can see this effect by having an app essentially pause all operation and then forcing a GC some time later. In the absence of heap pressure due to ongoing allocation, objects will not be cleared.

SoftReference is essentially just a WeakReference with a time delay from the last traversal. This is typically specified in time versus heap size, like milliseconds per megabyte. It can be tuned up or down, but the intent is that infrequently-accessed objects don't immediately get collected like you may see from WeakReference (and we have had bad tests come in from CRuby or ruby/spec that fail to consider this possibility).

Now, after 20+ reloads I don't see any more heap growth, other than those caused by soft references, which takes 1000s to be released by the jvm by default.

And it appears that all tests are passing, which is a pretty good testament. I will review the latest round of changes and see if I'm comfortable including this in JRuby 10.0.5.0 (which has been delayed until Monday to consider this and other issues).

I think this is the part that's confusing me:

ClassValue.newInstance() delegates to StableClassValue, which wraps java.lang.ClassValue. That stores computed values inside the Class object itself via JDK internals — not inside JavaSupport. So setting javaSupport = null makes our ClassValue instance unreachable from JRuby's side, but the JDK's own table entries (rooted at Class objects) can only be cleared by the GC's weak-key processing.

ClassValue does store the values in a WeakHashMap with the ClassValue's "identity" object used as the weak key. The references to values are hard, but dereferencing the ClassValue should allow these WeakHashMaps to dereference the ClassValue identities and eventually clear that hard reference.

So I believe I'm correct in thinking that clearing the JavaSupport reference is at least sufficient to ensure the proxy objects are vacated, but I recognize that may not happen as rapidly as we'd like (especially if there are hundreds or thousands of references from that ClassValue identity to our JavaClass proxies).

Using MapBasedClassValue gives us the ability to actively clear those references, since we control the underlying map, albeit at the cost of a much longer teardown process than simply walking awaay from them.

I'd ike to see what the heap curve looks like for each case. I suspect the regular ClassValue version will climb higher before clearing (due to WeakReference delays) but I would not expect it to continue "leaking" over time.

So I propose a compromise: ship the logic from your patch that actively clears MapBasedClassValue caches, but do not use MapBasedClassValue by default. This will allow us the benefit of ClassValue's JDK optimizations for non-embedded use cases (faster access and less teardown overhead for command-line apps) while allowing embedded use cases a way to opt into active ClassValue clearing with MapBasedClassValue.

It also minimizes the changes, since MapBasedClassValue is already configurable.

What do you think?

So I believe I'm correct in thinking that clearing the JavaSupport reference is at least sufficient to ensure the proxy objects are vacated, but I recognize that may not happen as rapidly as we'd like (especially if there are hundreds or thousands of references from that ClassValue identity to our JavaClass proxies).

Based on my test, no it is not sufficient.

I checked out jruby-10.0 and then only changed a single line in Ruby.java:

--- a/core/src/main/java/org/jruby/Ruby.java
+++ b/core/src/main/java/org/jruby/Ruby.java
@@ -3384,7 +3384,8 @@ public final class Ruby implements Constantizable {
         allModules.clear();
         constantNameInvalidators.clear();
         symbolTable.clear();
-        javaSupport = loadJavaSupport();
+        javaSupport = null;

Then I ran openhab with this JVM options:

-Xmx150m -XX:+UseG1GC -XX:G1PeriodicGCInterval=1000 -XX:-G1PeriodicGCInvokesConcurrent -XX:MinHeapFreeRatio=5 -XX:MaxHeapFreeRatio=10 -XX:SoftRefLRUPolicyMSPerMB=0

Then each time a new jruby engine is re-loaded I ran 5 GC calls to ensure that GC is performed.
It shows the same leak as the unmodified jruby-10.0.4.0 - in my case 9MB of heap is never reclaimed each time the engine reloads.

Given the small Xmx setting, on the 5th reload I eventually hit OOM:

java.lang.OutOfMemoryError: Java heap space
        at java.util.concurrent.ConcurrentHashMap$ValuesView.iterator(ConcurrentHashMap.java:4743) ~[?:?]
        at org.openhab.core.thing.internal.ThingManagerImpl.allEnabledThingsAreInitialized(ThingManagerImpl.java:1177) ~[?:?]
        at org.openhab.core.thing.internal.ThingManagerImpl.lambda$21(ThingManagerImpl.java:1208) ~[?:?]
        at org.openhab.core.thing.internal.ThingManagerImpl$$Lambda/0x0000000100e00228.run(Unknown Source) ~[?:?]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?]
        at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:358) ~[?:?]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) [?:?]
        at java.lang.Thread.runWith(Thread.java:1596) [?:?]
        at java.lang.Thread.run(Thread.java:1583) [?:?]

This seems to me that there is a genuine leak, and not merely SoftReferences waiting to be claimed.

@jimtng I think you misunderstood me. I meant that the ClassValue change should not be necessary if we are clearing the reference to JavaSupport, since that should in turn cause the ClassValues to all be dereferenced.

The other changes make sense.

Edit: DEreferenced

So I propose a compromise: ship the logic from your patch that actively clears MapBasedClassValue caches, but do not use MapBasedClassValue by default. This will allow us the benefit of ClassValue's JDK optimizations for non-embedded use cases (faster access and less teardown overhead for command-line apps) while allowing embedded use cases a way to opt into active ClassValue clearing with MapBasedClassValue.

I tried reverting all the MapBasedClassValue back to ClassValue as they were before this PR. The result is OOM on the 5th reload in the same set up as above. This shows me that we do need to use MapBasedClassValue and manually clear them.

This is way beyond my Java knowledge, so I asked copilot:

The answer is right there in the Java7ClassValue comment — this is the same known bug from #3228:

// If we put any objects that reference an org.jruby.Ruby runtime
// (like RubyClass instances) in here we run into a circular
// reference situation that GC isn't handling where they will
// always be strongly referenced and never garbage collected.
// This appears to be a bug in OpenJDK. See jruby/jruby#3228.

The exact cycle in StableClassValue (the current default)

StableClassValue stores StableValue objects in Class.classValueMap. StableValue is a non-static inner class of StableClassValue, so every instance carries an implicit StableClassValue.this reference:

String.class → classValueMap (WeakHashMap)
    weak key  ──────────────────────────────────────────── proxy.identity
    hard value → StableValue (inner class instance)
                   ↓ implicit outer ref
                   StableClassValue.this
                     → proxy (java.lang.ClassValue field)
                         → proxy.identity  ◄── SAME OBJECT as weak key
                   ↓ .result
                   RubyModule → Ruby_old → (javaSupport = null)

The hard value in the WeakHashMap strongly references proxy.identity, which IS the weak key. GC will never collect the weak key — it is permanently strongly reachable through the hard value. expungeStaleEntries() can run a million times and find nothing to remove. It doesn't matter how aggressive your GC flags are — this isn't a timing issue, it's a permanent strong reference to the weak key from within its own value.

Your GC flags, -SoftRefLRUPolicyMSPerMB=0, periodic full GC — none of them can help because the object the GC needs to collect is not actually weakly reachable. It has a strong reference to itself via the cycle.

How Java7ClassValue worked around this

It wrapped the stored value in WeakReference<T> to break the cycle:

private final java.lang.ClassValue<WeakReference<T>> proxy = ...

Now classValueMap stores a WeakReference<RubyModule> — that doesn't hold StableClassValue.this, so no cycle. The strong retention was moved into a List<T> computedValues inside proxy itself — which goes away when proxy goes away.

Why MapBasedClassValue is the right fix

MapBasedClassValue never writes into Class.classValueMap at all. There's no proxy.identity, no WeakHashMap with this pathological cycle, no JDK bug involved. cache.clear() is a direct ConcurrentHashMap.clear().

The bug is explicitly documented in JRuby's own Java7ClassValue as an OpenJDK bug (#3228), and StableClassValue (the current default) recreates the same cycle. MapBasedClassValue is not just an "eager clearing" preference — it's the only implementation that doesn't reproduce the known GC cycle bug.

Ok, give me a day to verify this and try to fix the ClassValue cycle.

I tried reverting all the MapBasedClassValue back to ClassValue as they were before this PR.

To clarify, I started with the current PR 7a17756 and only reverted the changes that involved the use of MapBasedClassValue back to their pre-PR ClassValue, but leaving all the other changes in this PR.

That didn't work, confirming that the changes to MapBasedClassValue are necessary.

I believe I've fixed the problem.

I'm using this script to reproduce, so hopefully I'm getting the same leaks as you are with OpenHAB:

loop {
  10.times {
    ruby = org.jruby.Ruby.newInstance
    ruby.evalScriptlet("java.lang.String; java.util.ArrayList; java.io.InputStream")
    ruby.tearDown
  }
  3.times { JRuby.gc }
  print "."
}

This script spins up 10 JRuby instances, accesses some Java classes to make sure they have been proxied, tears the instances down, and then does a few GCs.

Here's your PR as it stands with all changes in place, including MapBasedClassValue (about 4 minutes):

It's noisy, but level. Compare that to this graph, with the MapBasedClassValue changes reverted to ClassValue (a bit over 5 minutes):

The leak is obvious.

Copilot was right about one thing: the cycle is due to the StableClassValue.this from StableValue. But we can just patch StableClassValue without throwing it out.

Here's the patch:

diff --git a/core/src/main/java/org/jruby/util/collections/StableClassValue.java b/core/src/main/java/org/jruby/util/collections/StableClassValue.java
index 7d2a8999cf..2c78e0909a 100644
--- a/core/src/main/java/org/jruby/util/collections/StableClassValue.java
+++ b/core/src/main/java/org/jruby/util/collections/StableClassValue.java
@@ -7,6 +7,7 @@ import java.util.function.Function;
  * occurs at most once.
  */
 final class StableClassValue<T> extends ClassValue<T> {
+    final Object lock = new Object();
 
     public StableClassValue(ClassValueCalculator<T> calculator) {
         super(calculator);
@@ -27,11 +28,13 @@ final class StableClassValue<T> extends ClassValue<T> {
      * @param <Input> input of the computation
      * @param <Result> result of the computation
      */
-    private class StableValue<Input, Result> {
+    private static class StableValue<Input, Result> {
         private final Function<Input, Result> calculator;
         private volatile Result result;
-        StableValue(Function<Input, Result> calculator) {
+        private final Object lock;
+        StableValue(Object lock, Function<Input, Result> calculator) {
             this.calculator = calculator;
+            this.lock = lock;
         }
         Result get(Input input) {
             Result result = this.result;
@@ -39,7 +42,7 @@ final class StableClassValue<T> extends ClassValue<T> {
             if (result != null) return result;
 
             // lock on the StableClassValue so there are not multiple locks potentially in different orders
-            synchronized (StableClassValue.this) {
+            synchronized (lock) {
                 result = this.result;
 
                 if (result != null) return result;
@@ -55,7 +58,7 @@ final class StableClassValue<T> extends ClassValue<T> {
     private final java.lang.ClassValue<StableValue<Class<?>, T>> proxy = new java.lang.ClassValue<StableValue<Class<?>, T>>() {
         @Override
         protected StableValue<Class<?>, T> computeValue(Class<?> type) {
-            return new StableValue<>(calculator);
+            return new StableValue<>(lock, calculator);
         }
     };
 }

And here's the graph (20 minutes to be sure):

Noisy, but no worse than the Copilot version and no leak.

This is not exactly the patch I'd used; a proper Lock would be a better choice. But if you can confirm this fix with the MapBasedClassValue changes reverted, I'll do a cleaner version after we merge.

Wow nice find! I'll test in a few hours and report back.

Here's the patch:

diff --git a/core/src/main/java/org/jruby/util/collections/StableClassValue.java b/core/src/main/java/org/jruby/util/collections/StableClassValue.java
index 7d2a8999cf..2c78e0909a 100644
--- a/core/src/main/java/org/jruby/util/collections/StableClassValue.java
+++ b/core/src/main/java/org/jruby/util/collections/StableClassValue.java
@@ -7,6 +7,7 @@ import java.util.function.Function;
  * occurs at most once.
  */
 final class StableClassValue<T> extends ClassValue<T> {
+    final Object lock = new Object();
 
     public StableClassValue(ClassValueCalculator<T> calculator) {
         super(calculator);
@@ -27,11 +28,13 @@ final class StableClassValue<T> extends ClassValue<T> {
      * @param <Input> input of the computation
      * @param <Result> result of the computation
      */
-    private class StableValue<Input, Result> {
+    private static class StableValue<Input, Result> {
         private final Function<Input, Result> calculator;
         private volatile Result result;
-        StableValue(Function<Input, Result> calculator) {
+        private final Object lock;
+        StableValue(Object lock, Function<Input, Result> calculator) {
             this.calculator = calculator;
+            this.lock = lock;
         }
         Result get(Input input) {
             Result result = this.result;
@@ -39,7 +42,7 @@ final class StableClassValue<T> extends ClassValue<T> {
             if (result != null) return result;
 
             // lock on the StableClassValue so there are not multiple locks potentially in different orders
-            synchronized (StableClassValue.this) {
+            synchronized (lock) {
                 result = this.result;
 
                 if (result != null) return result;
@@ -55,7 +58,7 @@ final class StableClassValue<T> extends ClassValue<T> {
     private final java.lang.ClassValue<StableValue<Class<?>, T>> proxy = new java.lang.ClassValue<StableValue<Class<?>, T>>() {
         @Override
         protected StableValue<Class<?>, T> computeValue(Class<?> type) {
-            return new StableValue<>(calculator);
+            return new StableValue<>(lock, calculator);
         }
     };
 }

Unfortunately this didn't work for me.

• I reverted the MapBasedClassValue changes in JavaSupport.java and JavaSupportImpl.java
• I applied your diff to StableClassValue.java

Under my test, it also caused OOM crash after 5 reloads. I confirmed that the modified StableValue constructor was called by inserting a log call.

I tried asking copilot to go down this path and it tried setting calculator = null after it was used, but that too didn't fix the leak. This is what it's saying, FWIW:

The calculator = null fix was not sufficient. The remaining chain is even simpler and completely immune to GC — no amount of GC calls can break it.

The permanent strong chain

String.class  ← permanent GC root (bootstrap classloader, NEVER collected)
  └─► String.class.classValueMap (WeakHashMap)
           └─► backing array entry (hard value)
                 └─► StableValue_N
                       └─► .result  (hard field, volatile Result)
                             └─► RubyModule_N
                                   └─► .runtime
                                         └─► Ruby_N  ← NEVER collected

StableValue_N.result is a hard (strong) reference. String.class is a permanent GC root. Every class your script accesses (java.lang.String, java.util.ArrayList, etc.) creates one such chain. Ruby_N is permanently strongly reachable. GC cannot collect an object that is strongly reachable — that is literally the definition of "live object". Your 5 GC calls, G1PeriodicGCInterval, SoftRefLRUPolicyMSPerMB=0 — none of them have any effect on this.

Why headius's test hides the leak

His script only touches 3 JI classes (java.lang.String, java.util.ArrayList, java.io.InputStream). With 10 iterations, he has 30 StableValue.result entries pinning 10 Ruby instances. If each minimal Ruby instance is ~3-4MB, that's ~30-40MB retained — fits in 150MB. In a real application accessing hundreds or thousands of Java classes, each reload retains proportionally more.

Why Java7ClassValue actually worked

Worth noting — Java7ClassValue (the deprecated one) solved this correctly by storing WeakReference<T> in classValueMap rather than T directly:

// classValueMap hard value: WeakReference<RubyModule>  ← does NOT strongly hold Ruby_N
// strong reference to RubyModule_N lives in: proxy.computedValues (a List<T>)
//   but proxy is in the island {JavaSupport_N, proxy_N} which IS GC-eligible
//   after javaSupport = null → island collected → list dropped → RubyModule_N released

That approach actually worked. It was deprecated for performance/other reasons, not correctness.

Conclusion

StableClassValue with a hard result field simply cannot be used for runtime-scoped values that need to be released at teardown. MapBasedClassValue is the correct solution — it never writes into Class.classValueMap at all, so clear() is a direct ConcurrentHashMap.clear() with no involvement of permanent GC roots or java.lang.ClassValue mechanics.

This analysis is still wrong, or at least deeply misleading. My patience is wearing thing, but let's play this game a bit longer so we can get it right for 10.0.5.0.

AI is often confidently incorrect

His script only touches 3 JI classes (java.lang.String, java.util.ArrayList, java.io.InputStream). With 10 iterations, he has 30 StableValue.result entries pinning 10 Ruby instances.

For some reason, Copilot ignored completely that my script ran in a loop, generating and terminating 10 runtime instances and then running GC. Each iteration takes around 1-2 seconds on my system once warmed up.

After 5 minutes without my fix, it was consuming almost 500MB. The leak was obvious. Let's use that as a baseline.

Five minutes is 600 seconds. We'll be generous and say that 50MB out of that 500 was always going to be occupied, leaving 450MB leaking. That works out to something like 0.75MB leaking per second.

Now let's look at the fixed version that ran for 20 minutes. If it were still broken and every second leaked 0.75MB, that's 0.75MB * (1200s) = 900GB of heap memory that should have been permanently stuck.

You saw both graphs... where 5 minutes without the fix climbed up to 500MB, the fixed version stayed within the 100-200MB range. The total available heap size never even got past 500MB.

A leak was definitely fixed. It just wasn't the leak you saw when you tried to confirm it.

It's shocking that Copilot would be so wrong about this. It's confidently giving you bad information. Beware.

Java7ClassValue vs StableClassValue

I said above:

...with the MapBasedClassValue changes reverted to ClassValue

The changes I reverted dd not completely reset JavaSupport and JavaSupportImpl. I just removed the direct construction of MapBasedClassValue, leaving in place your earlier change to use the weakly-referenced Java7ClassValue via ClassValue.newWeakInstance.

Here's the diff I applied to revert those changes:

commit d88d692332ac4a1137f4532f6f627f84f128ba55
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Thu Apr 2 12:03:49 2026 -0500

    Revert MapBasedClassValue changes

diff --git a/core/src/main/java/org/jruby/javasupport/JavaSupport.java b/core/src/main/java/org/jruby/javasupport/JavaSupport.java
index 75089d1d6b..6bf9499314 100644
--- a/core/src/main/java/org/jruby/javasupport/JavaSupport.java
+++ b/core/src/main/java/org/jruby/javasupport/JavaSupport.java
@@ -67,8 +67,8 @@ public abstract class JavaSupport {
     protected final Ruby runtime;
 
     @Deprecated(since = "9.4.3.0")
-    private final MapBasedClassValue<JavaClass> javaClassCache;
-    private final MapBasedClassValue<RubyModule> proxyClassCache;
+    private final ClassValue<JavaClass> javaClassCache;
+    private final ClassValue<RubyModule> proxyClassCache;
 
     static final class UnfinishedProxy extends ReentrantLock {
         final RubyModule proxy;
@@ -113,9 +113,9 @@ public abstract class JavaSupport {
     public JavaSupport(final Ruby runtime) {
         this.runtime = runtime;
 
-        this.javaClassCache = new MapBasedClassValue<>(klass -> new JavaClass(runtime, getJavaClassClass(), klass));
+        this.javaClassCache = ClassValue.newWeakInstance(klass -> new JavaClass(runtime, getJavaClassClass(), klass));
 
-        this.proxyClassCache = new MapBasedClassValue<>(this::computeProxyClass);
+        this.proxyClassCache = ClassValue.newWeakInstance(this::computeProxyClass);
 
         // Proxy creation is synchronized (see above) so a HashMap is fine for recursion detection.
         this.unfinishedProxies = new ConcurrentHashMap<>(8, 0.75f, 1);
@@ -397,8 +397,8 @@ public abstract class JavaSupport {
     }
 
     public void tearDown() {
-        javaClassCache.clear();
-        proxyClassCache.clear();
+//        javaClassCache.clear();
+//        proxyClassCache.clear();
         unfinishedProxies.clear();
         objectProxyCache.clear();
     }
diff --git a/core/src/main/java/org/jruby/javasupport/JavaSupportImpl.java b/core/src/main/java/org/jruby/javasupport/JavaSupportImpl.java
index 21a81ae1b5..18738b1e54 100644
--- a/core/src/main/java/org/jruby/javasupport/JavaSupportImpl.java
+++ b/core/src/main/java/org/jruby/javasupport/JavaSupportImpl.java
@@ -53,10 +53,10 @@ import org.jruby.javasupport.proxy.JavaProxyClass;
  */
 public class JavaSupportImpl extends JavaSupport {
 
-    private final MapBasedClassValue<Map<String, AssignedName>> staticAssignedNames =
-            new MapBasedClassValue<>(JavaSupportImpl::newAssignedNames);
-    private final MapBasedClassValue<Map<String, AssignedName>> instanceAssignedNames =
-            new MapBasedClassValue<>(JavaSupportImpl::newAssignedNames);
+    private final ClassValue<Map<String, AssignedName>> staticAssignedNames =
+            ClassValue.newInstance(JavaSupportImpl::newAssignedNames);
+    private final ClassValue<Map<String, AssignedName>> instanceAssignedNames =
+            ClassValue.newInstance(JavaSupportImpl::newAssignedNames);
 
     public JavaSupportImpl(final Ruby runtime) {
         super(runtime);
@@ -138,8 +138,8 @@ public class JavaSupportImpl extends JavaSupport {
     public void tearDown() {
         super.tearDown();
 
-        staticAssignedNames.clear();
-        instanceAssignedNames.clear();
+//        staticAssignedNames.clear();
+//        instanceAssignedNames.clear();
 
         synchronized (javaProxyClasses) {
             javaProxyClasses.clear();
diff --git a/core/src/main/java/org/jruby/util/collections/ClassValue.java b/core/src/main/java/org/jruby/util/collections/ClassValue.java
index 6641aea029..170a1ae543 100644
--- a/core/src/main/java/org/jruby/util/collections/ClassValue.java
+++ b/core/src/main/java/org/jruby/util/collections/ClassValue.java
@@ -54,6 +54,17 @@ public abstract class ClassValue<T> {
         return Options.JI_CLASS_VALUES.load().function.apply(calculator);
     }
 
+    /**
+     * Create a weak-valued ClassValue for caches whose computed values can retain a Ruby runtime.
+     *
+     * java.lang.ClassValue has historically caused retention cycles in OpenJDK when values are
+     * strongly held and those values reference JRuby runtime state. Using weak values here breaks
+     * that cycle while preserving class-keyed lookup semantics.
+     */
+    public static <T> ClassValue<T> newWeakInstance(ClassValueCalculator<T> calculator) {
+        return new Java7ClassValue<>(calculator);
+    }
+
     @Deprecated(since = "9.4.13.0")
     private static <T> ClassValue<T> newJava7Instance(ClassValueCalculator<T> calculator) {
         return new Java7ClassValue<>(calculator);

I suspect this is the real difference between your most recent run and mine. Perhaps I should have been more specific and said "after reverting the MapBasedClassValue changes but retaining the newWeakInstance change" or simply provided my entire diff. In any case, you were testing something different than I was. We'll mark that down as a miscommunication aggravated by bad AI analysis.

It's notable that even though my run was using your weak proxy cache change, it still leaked 0.75MB per second. That's because the other two ClassValue in JavaSupportImpl went back to being StableClassValue (by default config) and that's likely where the retained memory came from.

Copilot was correct that the StableValue.this reference was a problem. My fix corrected that problem.. It seems to have ignored this and moved the goalposts to try to "help" you figure out why your test still failed.

The reference chain explanations... are correct. With all caches restored to StableClassValue, there is indeed a cycle from the classValueMap back to the StableClassValue.result, keeping it alive. That leaks much more quickly because it does actually retain a whole JRuby runtime (which is almost certainly much more than 3-4MB).

Fixing the right problem

There's still a memory issue when using StableClassValue to caching the proxy instances, that much we can agree on. The fix may be to return to weak references, or perhaps we can avoid some additional hidden references that are not cleared when the runtime is torn down. Throwing out the class altogether is not the answer I'm looking for.

In my last review, I asked of your MapBasedClassValue changes:

Is this change just so that we can go clear those entries? Again I wonder why dereferencing the JavaSupport is not enough to dereference the ClassValue and all values it refers to.

Let's drill down on this particular change.

If I go back to stock JavaSupport/Impl (from 10.0.4.0, with the newWeakInstance change also reverted) and run my example, it leaks very quickly because of StableClassValue retaining runtimes. This is after just one minute:

We can configure it to use MapBasedClassValue instead of StableValue by specifying JVM property ji.class.values=HARD_MAP.

Running the same script as above, but with jruby -Xji.class.values=HARD_MAP, memory usage remains stable. Copilot is correct that MapBasedClassValue avoids a GC root holding references to the JRuby runtimes. That's a big reason why it still exists.

Here's a 10-minute run just to be sure:

But here's the real kicker: my reverted JavaSupport/Impl does not include your MapBasedClassValue clearing logic. My local JavaSupport/Impl diff against 10.0.4.0 is empty. We have configured away the proxy leak solely by switching to MapBasedClassValue.

So the answer to "is this just so we can clear those entries?" appears to be "no", because MapBasedClassValue also avoids retaining runtime references from the bootstrap classloader GC root. That's why it is configurable.

More history

The use of ClassValue has gone through many iterations.

Originally, we tried to just use the JDK ClassValue directly, guarded by a configuration property (since we still supported earlier Java versions at the time). We made Java7ClassValue opt-in and provided MapBasedClassValue to abstract the API away.

Later, once we required Java 8 in JRuby 9k, we switched the default to Java7ClassValue. We quickly ran into the hard-referencing problem and a contributor helped us make Java7ClassValue hold weak references (#3228).

The current "breaking" change may have arrived last year when I started switching the default back to a regular JDK ClassValue. (5ac95ea in #8844). The torture tests I ran against this did not appear to leak.

Next, I discovered that ClassValue calculation was not idempotent and could execute more than once so I added StableClassValue and a configuration flag to select it (remaining changes in #8844).

At this point there were still no obvious leaks, as proved by a 12 hour run of my torture test. There clearly is a leak now.

So either my torture test was insufficient to show the leaks we are now seeing, or I later introduced another change that is causing the current leak.

That's the fix I currently seek.

The StableClassValue.this leak was introduced in 8288cac.

Well, how about that... the actual leak we seek to fix was also introduced in 8288cac.

Here's the fix (the StableClassValue lock fix is also needed):

diff --git a/core/src/main/java/org/jruby/javasupport/JavaSupport.java b/core/src/main/java/org/jruby/javasupport/JavaSupport.java
index e9c31e8778..150ee77ebb 100644
--- a/core/src/main/java/org/jruby/javasupport/JavaSupport.java
+++ b/core/src/main/java/org/jruby/javasupport/JavaSupport.java
@@ -114,21 +114,28 @@ public abstract class JavaSupport {
 
         this.javaClassCache = ClassValue.newInstance(klass -> new JavaClass(runtime, getJavaClassClass(), klass));
 
-        this.proxyClassCache = ClassValue.newInstance(this::computeProxyClass);
+        this.proxyClassCache = ClassValue.newInstance(new ProxyComputer(runtime)::computeProxyClass);
 
         // Proxy creation is synchronized (see above) so a HashMap is fine for recursion detection.
         this.unfinishedProxies = new ConcurrentHashMap<>(8, 0.75f, 1);
     }
 
-    /**
-     * Because of the complexity of processing a given class and all its dependencies,
-     * we opt to synchronize this logic. Creation of all proxies goes through here,
-     * allowing us to skip some threading work downstream.
-     */
-    private synchronized RubyModule computeProxyClass(Class<?> klass) {
-        RubyModule proxyKlass = Java.createProxyClassForClass(runtime, klass);
-        JavaExtensions.define(runtime, klass, proxyKlass); // (lazy) load extensions
-        return proxyKlass;
+    private static class ProxyComputer {
+        private final Ruby runtime;
+
+        ProxyComputer(Ruby runtime) {
+            this.runtime = runtime;
+        }
+        /**
+         * Because of the complexity of processing a given class and all its dependencies,
+         * we opt to synchronize this logic. Creation of all proxies goes through here,
+         * allowing us to skip some threading work downstream.
+         */
+        private synchronized RubyModule computeProxyClass(Class<?> klass) {
+            RubyModule proxyKlass = Java.createProxyClassForClass(runtime, klass);
+            JavaExtensions.define(runtime, klass, proxyKlass); // (lazy) load extensions
+            return proxyKlass;
+        }
     }

The problem was actually really simple: this::computeProxyValue is a lambda with a reference to JavaSupport, which references the ClassValue instances. Once I started looking for a long-lived reference from the ClassValue to itself, it was obvious.

Five minutes of monitoring seemed sufficient to prove it's fixed:

This is again not an ideal fix; the synchronization is now against the (singleton) ProxyComputer rather than against JavaSupport, so it won't disallow other JavaSupport synchronizers from running at the same time. It should be replaced with a shared lock that all such synchronizers use.

But it's probably good enough for now, and I'll clean it up with the rest of StableClassValue after we merge your other fixes.

@jimtng I don't want you to think I was impatient with you.

I know you were just trying to find a solution to a complex problem, and AI was the only tool you had to do it. Unfortunately in this case, it obscured the actual problem and led both of us around in circles.

I've lost a couple of days dealing with this misleading analysis, when I probably should have just tried to reproduce your case and investigated it myself. My impatience was with an AI telling me I was wrong when I wasn't and backing it up with falsehoods.

It's not your fault... you were just the middle man, and important information was getting lost along the way.

If you find that there's still memory being leaked, just let me know. Leave Copilot out of it.

Also, I take back what I said earlier. This analysis of the reference chain keeping everything alive was just wrong:

String.class  ← permanent GC root (bootstrap classloader, NEVER collected)
  └─► String.class.classValueMap (WeakHashMap)
           └─► backing array entry (hard value)
                 └─► StableValue_N
                       └─► .result  (hard field, volatile Result)
                             └─► RubyModule_N
                                   └─► .runtime
                                         └─► Ruby_N  ← NEVER collected

The bug I finally fixed involved a reference from ClassValue to the compute function to JavaSupport to the ClassValue. It was a simple sequence and the AI description of it was completely wrong.

In fact I'm pretty sure the only things that Copilot got right were:

• StableClassValue.this reference was keeping StableClassValue alive.
• There was still a leak after fixing that.

The rest was noise, or worse than noise since it convinced you that it had found the actual problem and made me doubt my own analysis and understanding of the system.

Fixes #9187 with my additions in place.

the synchronization is now against the (singleton) ProxyComputer rather than against JavaSupport, so it won't disallow other JavaSupport synchronizers from running at the same time. It should be replaced with a shared lock that all such synchronizers use.

But it's probably good enough for now, and I'll clean it up with the rest of StableClassValue after we merge your other fixes.

is it though? recall there was a reason to sync against concurrent proxy initialization due issues when multiple threads initializing the same proxy.

I'm going back over all the commits now because I no longer trust any of them. The most recent changes, which are clean and leak-free on my local system, fails a test that Copilot wrote. The first commit in the PR makes claims about a reference path that again seem to be incorrect. We won't be merging this as-is.

is it though? recall there was a reason to sync against concurrent proxy initialization due issues when multiple threads initializing the same proxy.

The synchronization against a global lock is necessary, but I'm not sure if that global lock needs to be the JavaSupport instance. In any case, I am going to redo this whole thing and make all places synchronizing this way use a common Lock instance.

Superseded by #9359