Bug 2016930
| Summary: | Class loader leaked due to ObjectStreamClass.lookup() | ||
|---|---|---|---|
| Product: | Red Hat Enterprise Linux 8 | Reporter: | Simeon Andreev <simeon.andreev> |
| Component: | java-11-openjdk | Assignee: | Roman Kennke <rkennke> |
| Status: | CLOSED CURRENTRELEASE | QA Contact: | OpenJDK QA <java-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | 8.4 | CC: | ahughes, fdemeloj, loskutov, mmillson, neugens |
| Target Milestone: | rc | Keywords: | Triaged |
| Target Release: | --- | Flags: | mmillson:
needinfo?
pm-rhel: mirror+ |
| Hardware: | x86_64 | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | java-11-openjdk-11.0.16.0.8-1.el7_9 | Doc Type: | If docs needed, set a value |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2022-08-04 16:30:30 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
In our application we unload a specific class loader, that loads user code, at controlled points in time. If the user code calls ObjectStreamClass.Caches (in the case we discovered, it was indirectly via "new org.junit.runner.Result.Result()"), class unloading does not occur "on time" and is delayed to a future GC. Our memory leak analysis fails due to this. This is the same issue me and Roman (rkennke) saw already last week and the conclusion was that this is indeed supposed to be a Soft Reference, not a Weak Reference. The suggestion was for the customer to use Shenandoah, or call twice or three times the GC to clean. He is not accepting any of the suggestions and he argues this is supposed to be a weak reference, and therefore this must be fixed. (In reply to Francisco De Melo from comment #6) > He is not accepting any of the suggestions and he argues this is supposed to > be a weak reference, and therefore this must be fixed. "He" is me and I honestly don't care if and how the code works internally, if it uses hard, soft or weak references or no references at all :-). Our expectation is: the code should not allow to produce memory leaks if used with default G1 GC and without loop with System.gc(). On a heap with ~12 GB System.gc() in a loop costs a lot and our application time costs real money because our application load very expensive HW where customers pay for time the HW is loaded. Currently our application has memory leak because of the mentioned code and we don't even have a workaround. If the solution would mean to introduce & use some extra system flag we would be also fine. We don't want & can't actually switch to a different GC because of performance issues with Shenandoah. (In reply to Andrey Loskutov from comment #7) > (In reply to Francisco De Melo from comment #6) > > He is not accepting any of the suggestions and he argues this is supposed to > > be a weak reference, and therefore this must be fixed. > > "He" is me and I honestly don't care if and how the code works internally, > if it uses hard, soft or weak references or no references at all :-). > Our expectation is: the code should not allow to produce memory leaks if > used with default G1 GC and without loop with System.gc(). > > On a heap with ~12 GB System.gc() in a loop costs a lot and our application > time costs real money because our application load very expensive HW where > customers pay for time the HW is loaded. > > Currently our application has memory leak because of the mentioned code and > we don't even have a workaround. > If the solution would mean to introduce & use some extra system flag we > would be also fine. > We don't want & can't actually switch to a different GC because of > performance issues with Shenandoah. First of all, strictly speaking, it is not a memory leak. The cache will eventually be cleared, and thus the class-loader be reclaimed, when heap gets under pressure. Apparently, with 12GB heap, it will take a while until the GC figures out that cleaning the cache would be reasonable. However, I agree that the behaviour is really not very useful, either. Very generally speaking, building a cache based on weak or soft references (doesn't matter, really) is asking for troubles, such as you observed. I am thinking that the caches should be implemented by a proper LRU cache. I will take care of it. Thanks, Roman Alright, I believe using SoftReference here is really non-sensical: the key in the map is a WeakReference to a Class, and the value is a SoftReference to an ObjectStreamClass, which itself also references the Class. That means, that the key will not be reclaimed as long as the Class in the value has been reclaimed, so that needs to go first. And that only happens when: 1. the corresponding ClassLoader gets reclaimed, 2. The GC determines that heap pressure is high enough to reclaim the SoftReference, and then it also needs to wait for the WeakReference to clear, after the SoftReference has gone. In any case, once the ClassLoader is gone, there is absolutely no point in retaining an entry in the cache, and vice versa: there is no reason to keep the entry in the cache, if there is no longer any way to reach the ClassLoader or the Class. So we can just as well use WeakReference instead of SoftReference for the value. I'm testing a fix and will report back. tracked upstream: https://bugs.openjdk.java.net/browse/JDK-8277072 Just found another reincarnation of the same problem, this time via using java.io.ObjectOutputStream.writeObject(Object) for serialization. Once we serialize something, unload the class loader and reload the classes again in the new one, we see same leak. java.io.ObjectOutputStream.writeObject(Object) calls into java.io.ObjectOutputStream.writeObject0(Object, boolean) and that calls into java.io.ObjectStreamClass.lookup(Class<?>, boolean) which is where the SoftReferences are created => same issue as seen here before. Regardless of *how* we introduce the memory leak, we've also found that we have a functional problem in our application, that affects interactive debugging. After the first leak is introduced, and we have 1 live + N (supposed to be unloaded) class loaders, we see that hot code replace starts to fail on classes with structural changes, because the same classes are still referenced by supposed to be unloaded classloaders. Note, "debugging" is not meant to be something special, our application implements a debugger for the user code and that is an essential part of the functionality we offer to users. Roman: could you share your patch (ideally for Java 11, but would be also OK for 17)? Do you have any insights if and when it could be merged into JDK? > Roman: could you share your patch (ideally for Java 11, but would be also OK > for 17)? > Do you have any insights if and when it could be merged into JDK? The fix, which turned out to be much more complex than the originally proposed change from SoftReference to WeakReference - which was wrong - has been integrated into JDK 19 branch just now: https://github.com/openjdk/jdk/pull/6375 After some bake-time, it will be backported to JDK 18, JDK 17, and eventually to JDK 11 (if accepted). Thanks, Roman |
Description of problem: We observe leaked class loaders due to code in Version-Release number of selected component (if applicable): openjdk version "11.0.10" 2021-01-19 LTS OpenJDK Runtime Environment 18.9 (build 11.0.10+9-LTS) OpenJDK 64-Bit Server VM 18.9 (build 11.0.10+9-LTS, mixed mode, sharing) How reproducible: Create a folder with the following classes, compile them and run the main class "TestObjectStreamClass": public class TestObjectStreamClass { public static void main(String[] args) throws Exception { TestClassLoader myOwnClassLoader = new TestClassLoader(); Class<?> loadClass = myOwnClassLoader.loadClass("ObjectStreamClass_MemoryLeakExample"); Object objectStreamClass_MemoryLeakExample = loadClass.newInstance(); objectStreamClass_MemoryLeakExample.toString(); java.lang.ref.WeakReference<Object> myOwnClassLoaderWeakReference = new java.lang.ref.WeakReference<>(myOwnClassLoader); System.out.println("weak reference before GC and setting to null: " + myOwnClassLoaderWeakReference.get()); objectStreamClass_MemoryLeakExample = null; myOwnClassLoader = null; loadClass = null; System.out.println("weak reference before GC: " + myOwnClassLoaderWeakReference.get()); gc(); System.out.println("weak reference after GC: " + myOwnClassLoaderWeakReference.get()); } public static void gc() { System.runFinalization(); System.gc(); } } public 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) { java.io.ByteArrayOutputStream byteSt = new java.io.ByteArrayOutputStream(); try ( java.io.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(); } } public class ObjectStreamClass_MemoryLeakExample { private static final java.io.ObjectStreamField[] fields = java.io.ObjectStreamClass.lookup(TestClass.class).getFields(); @Override public String toString() { return java.util.Arrays.toString(fields); } } public class TestClass implements java.io.Serializable { public String x; } rm *.class ; /usr/lib/jvm/java-11/bin/javac *.java && /usr/lib/jvm/java-11/bin/java TestObjectStreamClass Observe output: Note: TestObjectStreamClass.java uses or overrides a deprecated API. Note: Recompile with -Xlint:deprecation for details. weak reference before GC and setting to null: TestClassLoader@6ff3c5b5 weak reference before GC: TestClassLoader@6ff3c5b5 weak reference after GC: TestClassLoader@6ff3c5b5 If call to "java.io.ObjectStreamClass.lookup()" is commented out (i.e. the static field is initialized with a null), the weak reference is cleared as expected: Note: TestObjectStreamClass.java uses or overrides a deprecated API. Note: Recompile with -Xlint:deprecation for details. weak reference before GC and setting to null: TestClassLoader@6ff3c5b5 weak reference before GC: TestClassLoader@6ff3c5b5 weak reference after GC: null Actual results: Weak reference is not cleared, indicating the class loader was leaked. Expected results: Class loader is not leaked. Additional info: See related OpenJDK bug: https://bugs.openjdk.java.net/browse/JDK-8199589 According to the colleague who analysed the issue, the problem is with "localDescs" and "reflectors" of the ObjectStreamClass.Caches class: private static class Caches { /** cache mapping local classes -> descriptors */ static final ConcurrentMap<WeakClassKey,Reference<?>> localDescs = new ConcurrentHashMap<>(); /** cache mapping field group/local desc pairs -> field reflectors */ static final ConcurrentMap<FieldReflectorKey,Reference<?>> reflectors = new ConcurrentHashMap<>(); /** queue for WeakReferences to local classes */ private static final ReferenceQueue<Class<?>> localDescsQueue = new ReferenceQueue<>(); /** queue for WeakReferences to field reflectors keys */ private static final ReferenceQueue<Class<?>> reflectorsQueue = new ReferenceQueue<>(); } Those are apparently soft references and not weak references.