8277072: ObjectStreamClass caches keep ClassLoaders alive

Reviewed-by: rriggs, plevart

src/java.base/share/classes/java/io/ClassCache.java

@@ -0,0 +1,87 @@
+/*
+ * Copyright (c) 2021, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package java.io;
+
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.SoftReference;
+
+// Maps Class instances to values of type T. Under memory pressure, the
+// mapping is released (under soft references GC policy) and would be
+// recomputed the next time it is queried. The mapping is bound to the
+// lifetime of the class: when the class is unloaded, the mapping is
+// removed too.
+abstract class ClassCache<T> {
+
+    private static class CacheRef<T> extends SoftReference<T> {
+        private final Class<?> type;
+
+        CacheRef(T referent, ReferenceQueue<T> queue, Class<?> type) {
+            super(referent, queue);
+            this.type = type;
+        }
+
+        Class<?> getType() {
+            return type;
+        }
+    }
+
+    private final ReferenceQueue<T> queue;
+    private final ClassValue<SoftReference<T>> map;
+
+    protected abstract T computeValue(Class<?> cl);
+
+    protected ClassCache() {
+        queue = new ReferenceQueue<>();
+        map = new ClassValue<>() {
+            @Override
+            protected SoftReference<T> computeValue(Class<?> type) {
+                return new CacheRef<>(ClassCache.this.computeValue(type), queue, type);
+            }
+        };
+    }
+
+    T get(Class<?> cl) {
+        processQueue();
+        T val;
+        do {
+            SoftReference<T> ref = map.get(cl);
+            val = ref.get();
+            if (val == null) {
+                map.remove(cl);
+            }
+        } while (val == null);
+        return val;
+    }
+
+    private void processQueue() {
+        Reference<? extends T> ref;
+        while((ref = queue.poll()) != null) {
+            CacheRef<? extends T> cacheRef = (CacheRef<? extends T>)ref;
+            map.remove(cacheRef.getType());
+        }
+    }
+}

src/java.base/share/classes/java/io/ObjectStreamClass.java

@@ -30,7 +30,6 @@ import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
 import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
-import java.lang.ref.SoftReference;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
@@ -108,19 +107,22 @@ public class ObjectStreamClass implements Serializable {
 
     private static class Caches {
         /** cache mapping local classes -> descriptors */
-        static final ConcurrentMap<WeakClassKey,Reference<?>> localDescs =
+        static final ClassCache<ObjectStreamClass> localDescs =
-            new ConcurrentHashMap<>();
+            new ClassCache<>() {
+                @Override
+                protected ObjectStreamClass computeValue(Class<?> type) {
+                    return new ObjectStreamClass(type);
+                }
+            };
 
         /** cache mapping field group/local desc pairs -> field reflectors */
-        static final ConcurrentMap<FieldReflectorKey,Reference<?>> reflectors =
+        static final ClassCache<Map<FieldReflectorKey, FieldReflector>> reflectors =
-            new ConcurrentHashMap<>();
+            new ClassCache<>() {
-
+                @Override
-        /** queue for WeakReferences to local classes */
+                protected Map<FieldReflectorKey, FieldReflector> computeValue(Class<?> type) {
-        private static final ReferenceQueue<Class<?>> localDescsQueue =
+                    return new ConcurrentHashMap<>();
-            new ReferenceQueue<>();
+                }
-        /** queue for WeakReferences to field reflectors keys */
+            };
-        private static final ReferenceQueue<Class<?>> reflectorsQueue =
-            new ReferenceQueue<>();
     }
 
     /** class associated with this descriptor (if any) */
@@ -362,136 +364,7 @@ public class ObjectStreamClass implements Serializable {
         if (!(all || Serializable.class.isAssignableFrom(cl))) {
             return null;
         }
-        processQueue(Caches.localDescsQueue, Caches.localDescs);
+        return Caches.localDescs.get(cl);
-        WeakClassKey key = new WeakClassKey(cl, Caches.localDescsQueue);
-        Reference<?> ref = Caches.localDescs.get(key);
-        Object entry = null;
-        if (ref != null) {
-            entry = ref.get();
-        }
-        EntryFuture future = null;
-        if (entry == null) {
-            EntryFuture newEntry = new EntryFuture();
-            Reference<?> newRef = new SoftReference<>(newEntry);
-            do {
-                if (ref != null) {
-                    Caches.localDescs.remove(key, ref);
-                }
-                ref = Caches.localDescs.putIfAbsent(key, newRef);
-                if (ref != null) {
-                    entry = ref.get();
-                }
-            } while (ref != null && entry == null);
-            if (entry == null) {
-                future = newEntry;
-            }
-        }
-
-        if (entry instanceof ObjectStreamClass) {  // check common case first
-            return (ObjectStreamClass) entry;
-        }
-        if (entry instanceof EntryFuture) {
-            future = (EntryFuture) entry;
-            if (future.getOwner() == Thread.currentThread()) {
-                /*
-                 * Handle nested call situation described by 4803747: waiting
-                 * for future value to be set by a lookup() call further up the
-                 * stack will result in deadlock, so calculate and set the
-                 * future value here instead.
-                 */
-                entry = null;
-            } else {
-                entry = future.get();
-            }
-        }
-        if (entry == null) {
-            try {
-                entry = new ObjectStreamClass(cl);
-            } catch (Throwable th) {
-                entry = th;
-            }
-            if (future.set(entry)) {
-                Caches.localDescs.put(key, new SoftReference<>(entry));
-            } else {
-                // nested lookup call already set future
-                entry = future.get();
-            }
-        }
-
-        if (entry instanceof ObjectStreamClass) {
-            return (ObjectStreamClass) entry;
-        } else if (entry instanceof RuntimeException) {
-            throw (RuntimeException) entry;
-        } else if (entry instanceof Error) {
-            throw (Error) entry;
-        } else {
-            throw new InternalError("unexpected entry: " + entry);
-        }
-    }
-
-    /**
-     * Placeholder used in class descriptor and field reflector lookup tables
-     * for an entry in the process of being initialized.  (Internal) callers
-     * which receive an EntryFuture belonging to another thread as the result
-     * of a lookup should call the get() method of the EntryFuture; this will
-     * return the actual entry once it is ready for use and has been set().  To
-     * conserve objects, EntryFutures synchronize on themselves.
-     */
-    private static class EntryFuture {
-
-        private static final Object unset = new Object();
-        private final Thread owner = Thread.currentThread();
-        private Object entry = unset;
-
-        /**
-         * Attempts to set the value contained by this EntryFuture.  If the
-         * EntryFuture's value has not been set already, then the value is
-         * saved, any callers blocked in the get() method are notified, and
-         * true is returned.  If the value has already been set, then no saving
-         * or notification occurs, and false is returned.
-         */
-        synchronized boolean set(Object entry) {
-            if (this.entry != unset) {
-                return false;
-            }
-            this.entry = entry;
-            notifyAll();
-            return true;
-        }
-
-        /**
-         * Returns the value contained by this EntryFuture, blocking if
-         * necessary until a value is set.
-         */
-        @SuppressWarnings("removal")
-        synchronized Object get() {
-            boolean interrupted = false;
-            while (entry == unset) {
-                try {
-                    wait();
-                } catch (InterruptedException ex) {
-                    interrupted = true;
-                }
-            }
-            if (interrupted) {
-                AccessController.doPrivileged(
-                    new PrivilegedAction<>() {
-                        public Void run() {
-                            Thread.currentThread().interrupt();
-                            return null;
-                        }
-                    }
-                );
-            }
-            return entry;
-        }
-
-        /**
-         * Returns the thread that created this EntryFuture.
-         */
-        Thread getOwner() {
-            return owner;
-        }
     }
 
     /**
@@ -2248,82 +2121,39 @@ public class ObjectStreamClass implements Serializable {
     {
         // class irrelevant if no fields
         Class<?> cl = (localDesc != null && fields.length > 0) ?
-            localDesc.cl : null;
+            localDesc.cl : Void.class;
-        processQueue(Caches.reflectorsQueue, Caches.reflectors);
-        FieldReflectorKey key = new FieldReflectorKey(cl, fields,
-                                                      Caches.reflectorsQueue);
-        Reference<?> ref = Caches.reflectors.get(key);
-        Object entry = null;
-        if (ref != null) {
-            entry = ref.get();
-        }
-        EntryFuture future = null;
-        if (entry == null) {
-            EntryFuture newEntry = new EntryFuture();
-            Reference<?> newRef = new SoftReference<>(newEntry);
-            do {
-                if (ref != null) {
-                    Caches.reflectors.remove(key, ref);
-                }
-                ref = Caches.reflectors.putIfAbsent(key, newRef);
-                if (ref != null) {
-                    entry = ref.get();
-                }
-            } while (ref != null && entry == null);
-            if (entry == null) {
-                future = newEntry;
-            }
-        }
 
-        if (entry instanceof FieldReflector) {  // check common case first
+        var clReflectors = Caches.reflectors.get(cl);
-            return (FieldReflector) entry;
+        var key = new FieldReflectorKey(fields);
-        } else if (entry instanceof EntryFuture) {
+        var reflector = clReflectors.get(key);
-            entry = ((EntryFuture) entry).get();
+        if (reflector == null) {
-        } else if (entry == null) {
+            reflector = new FieldReflector(matchFields(fields, localDesc));
-            try {
+            var oldReflector = clReflectors.putIfAbsent(key, reflector);
-                entry = new FieldReflector(matchFields(fields, localDesc));
+            if (oldReflector != null) {
-            } catch (Throwable th) {
+                reflector = oldReflector;
-                entry = th;
             }
-            future.set(entry);
-            Caches.reflectors.put(key, new SoftReference<>(entry));
-        }
-
-        if (entry instanceof FieldReflector) {
-            return (FieldReflector) entry;
-        } else if (entry instanceof InvalidClassException) {
-            throw (InvalidClassException) entry;
-        } else if (entry instanceof RuntimeException) {
-            throw (RuntimeException) entry;
-        } else if (entry instanceof Error) {
-            throw (Error) entry;
-        } else {
-            throw new InternalError("unexpected entry: " + entry);
         }
+        return reflector;
     }
 
     /**
      * FieldReflector cache lookup key.  Keys are considered equal if they
-     * refer to the same class and equivalent field formats.
+     * refer to equivalent field formats.
      */
-    private static class FieldReflectorKey extends WeakReference<Class<?>> {
+    private static class FieldReflectorKey {
 
         private final String[] sigs;
         private final int hash;
-        private final boolean nullClass;
 
-        FieldReflectorKey(Class<?> cl, ObjectStreamField[] fields,
+        FieldReflectorKey(ObjectStreamField[] fields)
-                          ReferenceQueue<Class<?>> queue)
         {
-            super(cl, queue);
-            nullClass = (cl == null);
             sigs = new String[2 * fields.length];
             for (int i = 0, j = 0; i < fields.length; i++) {
                 ObjectStreamField f = fields[i];
                 sigs[j++] = f.getName();
                 sigs[j++] = f.getSignature();
             }
-            hash = System.identityHashCode(cl) + Arrays.hashCode(sigs);
+            hash = Arrays.hashCode(sigs);
         }
 
         public int hashCode() {
@@ -2331,19 +2161,9 @@ public class ObjectStreamClass implements Serializable {
         }
 
         public boolean equals(Object obj) {
-            if (obj == this) {
+            return obj == this ||
-                return true;
+                   obj instanceof FieldReflectorKey other &&
-            }
-
-            if (obj instanceof FieldReflectorKey other) {
-                Class<?> referent;
-                return (nullClass ? other.nullClass
-                                  : ((referent = get()) != null) &&
-                                    (other.refersTo(referent))) &&
                    Arrays.equals(sigs, other.sigs);
-            } else {
-                return false;
-            }
         }
     }
 

test/jdk/java/io/ObjectStreamClass/ObjectStreamClassCaching.java

@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2021, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.lang.ref.Reference;
+import java.lang.ref.WeakReference;
+import java.io.ObjectStreamClass;
+import java.io.Serializable;
+import java.util.ArrayList;
+import org.testng.annotations.Test;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+/* @test
+ * @bug 8277072
+ * @library /test/lib/
+ * @summary ObjectStreamClass caches keep ClassLoaders alive
+ * @run testng/othervm -Xmx10m -XX:SoftRefLRUPolicyMSPerMB=1 ObjectStreamClassCaching
+ */
+public class ObjectStreamClassCaching {
+
+    @Test
+    public void testCachingEffectiveness() throws Exception {
+        var ref = lookupObjectStreamClass(TestClass.class);
+        System.gc();
+        Thread.sleep(100L);
+        // to trigger any ReferenceQueue processing...
+        lookupObjectStreamClass(AnotherTestClass.class);
+        assertFalse(ref.refersTo(null),
+                    "Cache lost entry although memory was not under pressure");
+    }
+
+    @Test
+    public void testCacheReleaseUnderMemoryPressure() throws Exception {
+        var ref = lookupObjectStreamClass(TestClass.class);
+        pressMemoryHard(ref);
+        System.gc();
+        Thread.sleep(100L);
+        assertTrue(ref.refersTo(null),
+                   "Cache still has entry although memory was pressed hard");
+    }
+
+    // separate method so that the looked-up ObjectStreamClass is not kept on stack
+    private static WeakReference<?> lookupObjectStreamClass(Class<?> cl) {
+        return new WeakReference<>(ObjectStreamClass.lookup(cl));
+    }
+
+    private static void pressMemoryHard(Reference<?> ref) {
+        try {
+            var list = new ArrayList<>();
+            while (!ref.refersTo(null)) {
+                list.add(new byte[1024 * 1024 * 64]); // 64 MiB chunks
+            }
+        } catch (OutOfMemoryError e) {
+            // release
+        }
+    }
+}
+
+class TestClass implements Serializable {
+}
+
+class AnotherTestClass implements Serializable {
+}

test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java

@@ -0,0 +1,108 @@
+/*
+ * Copyright (c) 2021, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.lang.ref.WeakReference;
+import java.lang.reflect.Constructor;
+import java.io.ByteArrayOutputStream;
+import java.io.InputStream;
+import java.io.ObjectStreamClass;
+import java.io.ObjectStreamField;
+import java.io.Serializable;
+import java.util.Arrays;
+import org.testng.annotations.Test;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+
+import jdk.test.lib.util.ForceGC;
+
+/* @test
+ * @bug 8277072
+ * @library /test/lib/
+ * @build jdk.test.lib.util.ForceGC
+ * @summary ObjectStreamClass caches keep ClassLoaders alive
+ * @run testng TestOSCClassLoaderLeak
+ */
+public class TestOSCClassLoaderLeak {
+
+    @Test
+    public void testClassLoaderLeak() throws Exception {
+        TestClassLoader myOwnClassLoader = new TestClassLoader();
+        Class<?> loadClass = myOwnClassLoader.loadClass("ObjectStreamClass_MemoryLeakExample");
+        Constructor con = loadClass.getConstructor();
+        con.setAccessible(true);
+        Object objectStreamClass_MemoryLeakExample = con.newInstance();
+        objectStreamClass_MemoryLeakExample.toString();
+
+        WeakReference<Object> myOwnClassLoaderWeakReference = new WeakReference<>(myOwnClassLoader);
+        assertNotNull(myOwnClassLoaderWeakReference.get());
+        objectStreamClass_MemoryLeakExample = null;
+        myOwnClassLoader = null;
+        loadClass = null;
+        con = null;
+        assertNotNull(myOwnClassLoaderWeakReference.get());
+
+        ForceGC gc = new ForceGC();
+        assertTrue(gc.await(() -> myOwnClassLoaderWeakReference.get() == null));
+    }
+}
+
+class ObjectStreamClass_MemoryLeakExample {
+    private static final ObjectStreamField[] fields = ObjectStreamClass.lookup(TestClass.class).getFields();
+    public ObjectStreamClass_MemoryLeakExample() {
+    }
+
+    @Override
+    public String toString() {
+        return Arrays.toString(fields);
+    }
+}
+
+class TestClassLoader extends ClassLoader {
+
+    @Override
+    public Class<?> loadClass(String name) throws ClassNotFoundException {
+        if (name.equals("TestClass") || name.equals("ObjectStreamClass_MemoryLeakExample")) {
+            byte[] bt = loadClassData(name);
+            return defineClass(name, bt, 0, bt.length);
+        } else {
+            return super.loadClass(name);
+        }
+    }
+
+    private static byte[] loadClassData(String className) {
+        ByteArrayOutputStream byteSt = new ByteArrayOutputStream();
+        try (InputStream is = TestClassLoader.class.getClassLoader().getResourceAsStream(className.replace(".", "/") + ".class")) {
+            int len = 0;
+            while ((len = is.read()) != -1) {
+                byteSt.write(len);
+            }
+        } catch (java.io.IOException e) {
+            e.printStackTrace();
+        }
+        return byteSt.toByteArray();
+    }
+}
+
+class TestClass implements Serializable {
+    public String x;
+}