Custom classloader environment exposes GC class/loader leak in ClassByNameCache

[chrisdennis]

The implementation of ClassByNameCache appears to cause class leaks by failing to properly clean up FutureValue references on the failure path where the loader is not "the system loader". This seems to cause two symptoms:

1. Singular classes (highly unlikely to be defined multiple times) that are loaded by the either the platform loader, or the bootstrap loader are left in the cache, indirecting through FutureValue instances on every lookup. This is harmless but is probably costing some tiny amount of performance:

2. Classes loaded by custom class loaders that need to be unloadable cannot be since the ClassByNameCache is strongly referencing one of the loaded classes that has been pulled through serialization.

The trivial fix to 2 is purge the cache key on seeing a "non-system loader" in the update method:

diff --git a/closed/src/java.base/share/classes/java/io/ClassByNameCache.java b/closed/src/java.base/share/classes/java/io/ClassByNameCache.java
index 332caec9675..e8e3801c145 100644
--- a/closed/src/java.base/share/classes/java/io/ClassByNameCache.java
+++ b/closed/src/java.base/share/classes/java/io/ClassByNameCache.java
@@ -116,6 +116,7 @@ void update(CacheKey key, Class<?> result) {
         Object resultLoaderObj =
             LoaderRef.getLoaderObj(result.getClassLoader());
         if (getCanonicalLoaderRef(resultLoaderObj).isSystem == false) {
+            cache.remove(key);
             return;
         }
 

This will have the unfortunate side-effect of negating the performance benefit of the class caching for the bulk of the JDK standard library classes. A complete fix would be to also enhance the loader check to take in to account all three of the "special" loaders: system, platform and bootstrap.

[pshipton]

@tajila @hangshao0 fyi

[tajila]

@thallium Please take a look

[chrisdennis]

While this is being looked at we're also leaking FutureValue's on a Class.forName(...) lookup failure. I'm not sure this is actually harmful... beyond a tiny leak.

Coincidentally I've seen another developer in an adjacent team to me get a "false failure" - a CNFE presumably due to an incorrect loader reference that goes away when the caching is disabled. This feels to me like either:

1. A false cache hit on a previously failed lookup that used a different loader - but I see no obvious bugs in the cache lookup logic by inspection.
2. Some kind of weird error in the evaluation of latestUserDefinedLoader?
3. or a flaw in the compiler transformation optimization that evaluates the correct loader for a given call path?

[keithc-ca]

The map ClassByNameCache.canonicalLoaderRefs will only be non-empty if assertions are enabled. I don't think that was the intent.

  private void setCanonicalSystemLoaderRef(java.lang.ClassLoader);
    Code:
         0: new           #58                 // class java/io/ClassByNameCache$LoaderRef
         3: dup
         4: aload_1
         5: aload_0
         6: getfield      #25                 // Field staleLoaderRefs:Ljava/lang/ref/ReferenceQueue;
         9: iconst_1
        10: invokespecial #60                 // Method java/io/ClassByNameCache$LoaderRef."<init>":(Ljava/lang/ClassLoader;Ljava/lang/ref/ReferenceQueue;Z)V
        13: astore_2
        14: getstatic     #63                 // Field $assertionsDisabled:Z
        17: ifne          40
        20: aload_0
        21: getfield      #19                 // Field canonicalLoaderRefs:Ljava/util/concurrent/ConcurrentHashMap;
        24: aload_2
        25: aload_2
        26: invokevirtual #67                 // Method java/util/concurrent/ConcurrentHashMap.put:(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
        29: ifnull        40
        32: new           #71                 // class java/lang/AssertionError
        35: dup
        36: invokespecial #73                 // Method java/lang/AssertionError."<init>":()V
        39: athrow
        40: return

[chrisdennis]

Yes... this is unfortunate:

assert(canonicalLoaderRefs.put(newKey, newKey) == null);

At least the second problem is an easy fix.

[chrisdennis]

Coincidentally I've seen another developer in an adjacent team to me get a "false failure" - a CNFE presumably due to an incorrect loader reference that goes away when the caching is disabled.

I believe this is a bug in the caching of the LUDCL. I plan to dig more to see if a simpler test case can be derived - I will then file a separate issue once I have sufficient detail.

[thallium]

Hi @chrisdennis, do you have an easy way to reproduce this issue?

[chrisdennis]

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.lang.ref.PhantomReference;
import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Paths;
import java.util.concurrent.TimeUnit;

public class SerializationLeak {

    public static void main(String[] args) throws Exception {
        ReferenceQueue<ClassLoader> queue = new ReferenceQueue<>();
        
        PhantomReference<ClassLoader> reference = isolate(queue);

        long end = System.nanoTime() + TimeUnit.SECONDS.toNanos(10);
        while(System.nanoTime() < end) {
            if (queue.poll() == null) {
                System.gc();
            } else {
                System.out.println("ClassLoader GCed");
                break;
            }
        }
        Reference.reachabilityFence(reference);
    }



    private static PhantomReference<ClassLoader> isolate(ReferenceQueue<ClassLoader> queue) throws Exception  {
        ClassLoader classLoader = new URLClassLoader(new URL[] { Paths.get(".").toUri().toURL() }, ClassLoader.getPlatformClassLoader());

        PhantomReference<ClassLoader> classloaderRef = new PhantomReference<>(classLoader, queue);

        classLoader.loadClass("SerializationLeak$IsolatedClass").getMethod("serialization").invoke(null);

        return classloaderRef;
    }

    public static class IsolatedClass implements Serializable {
        
        public static void serialization() throws Exception {
            ByteArrayOutputStream bout = new ByteArrayOutputStream();
            try (ObjectOutputStream oout = new ObjectOutputStream(bout)) {
                oout.writeObject(new IsolatedClass());
            }

            try (ByteArrayInputStream bin = new ByteArrayInputStream(bout.toByteArray());
                ObjectInputStream oin = new ObjectInputStream(bin)) {
                oin.readObject();
            } 
        }
    }

}

Results:

$ openjdk-21.jdk/Contents/Home/bin/java -showversion SerializationLeak                                           
openjdk version "21.0.8" 2025-07-15
OpenJDK Runtime Environment Homebrew (build 21.0.8)
OpenJDK 64-Bit Server VM Homebrew (build 21.0.8, mixed mode, sharing)
ClassLoader GCed
$ ibm-semeru-open-21.jdk/Contents/Home/bin/java -showversion -Dcom.ibm.enableClassCaching=false SerializationLeak
openjdk version "21.0.7" 2025-04-15 LTS
IBM Semeru Runtime Open Edition 21.0.7.0 (build 21.0.7+6-LTS)
Eclipse OpenJ9 VM 21.0.7.0 (build openj9-0.51.0, JRE 21 Mac OS X aarch64-64-Bit 20250415_442 (JIT enabled, AOT enabled)
OpenJ9   - 31cf5538b0
OMR      - 9bcff94a2
JCL      - 26c2dc3d801 based on jdk-21.0.7+6)
ClassLoader GCed
$ ibm-semeru-open-21.jdk/Contents/Home/bin/java -showversion SerializationLeak
openjdk version "21.0.7" 2025-04-15 LTS
IBM Semeru Runtime Open Edition 21.0.7.0 (build 21.0.7+6-LTS)
Eclipse OpenJ9 VM 21.0.7.0 (build openj9-0.51.0, JRE 21 Mac OS X aarch64-64-Bit 20250415_442 (JIT enabled, AOT enabled)
OpenJ9   - 31cf5538b0
OMR      - 9bcff94a2
JCL      - 26c2dc3d801 based on jdk-21.0.7+6)

[thallium]

The solution that I have in mind is to replace the value field in FutureValue with a weak reference, i.e. WeakReference<Class<?>> value. @amicic @dmitripivkine do you know if this change has any potential impact on GC performance? FYI this issue is mainly about the cached value i.e. FutureValue holding a strong reference to the class which prevents the classloader to be unloaded. Please let me know if any other context needs to be explained.

[tajila]

@thallium I think you included the wrong Aleks @amicic

[chrisdennis]

My inference from the design as it stands is that FutureValue instances are only supposed to exist transiently and so it shouldn't matter that they strongly reference their classes. The intent here is to avoid duplicate lookups if a bunch of threads all arrive wanting to lookup the same class. Instead of everyone doing their own lookup, one of them wins the race to place the FutureValue, then another one wins the race to enter the synchronized block and perform a lookup. Everyone else waits at the monitor entry and then picks up the cached result.

All that's missing from the implementation is a bunch of FutureValue cleanup on some exit paths out of the code. One path is the isSystem == false path, and the other is the CNFE path. Close those two holes and move the system cache loader put outside of the assert and you should be fine.

[dmitripivkine]

The solution that I have in mind is to replace the value field in FutureValue with a weak reference, i.e. WeakReference<Class<?>> value. Do you know if this change has any potential impact on GC performance? FYI this issue is mainly about the cached value i.e. FutureValue holding a strong reference to the class which prevents the classloader to be unloaded. Please let me know if any other context needs to be explained.

I don't understand context and scale of the change nor is it right way to fix the problem. Using single Weak Reference as such has little impact on GC performance. Any solution should pass performance testing.

[thallium]

One path is the isSystem == false path

I don't understand why this path is an exit path?

[chrisdennis]

Not sure what you mean, but assuming you mean 'why' we exit out early for classes with "custom" class loaders, for good or ill based on the class javadoc I don't think was ever intended that classes loaded by custom class loaders would be cached here:

 *  Caching is done only when the actually used loader for a Class is one of the Sytem loaders (ie) App Class Loader,
 *  System Extension loader and BootStrap loader.