Dead lock on class loader #54
Conversation
|
@marchof do you have any thoughts about this ticket? |
|
@marchof I can't find released version of agent with package |
|
Might be related to https://bugs.openjdk.java.net/browse/JDK-8140477 , which contains traces of JaCoCo version 0.7.4. |
|
ClassLoader.loadClass locks itself then delegates to parent, which does the same, so class loader locking order must be child then parent to avoid deadlocks. AppClassLoader delegates to ExtClassLoader, which delegates to boot class loader, so threads must acquire the lock on AppClassLoader then on ExtClassLoader to avoid deadlocks. Also note that -javaagent classes are loaded by AppClassLoader. Thread-20 has attempted to load a class through AppClassLoader, so AppClassLoader locks itself as normal and then delegates to ExtClassLoader, which attempts to lock itself. pool-tool-thread-1 is attempting to load a class through ExtClassLoader, so ExtClassLoader locks itself as normal, and then the JaCoCo transformer is called. The JaCoCo transformer requires a new class to be loaded from its class loader (AppClassLoader), which attempts to lock AppClassLoader itself. This thread is attempting to acquire locks in the wrong order, so the deadlock occurs. To completely avoid the deadlock, I think either JaCoCo instrumentation needs to be disabled for classes loaded by any parents of AppClassLoader, so bootstrap class loader or ExtClassLoader (e.g., add inclextclasses agent option that defaults to false ala inclbootstrapclasses), or JaCoCo needs to preload all classes (e.g., during premain) that it might need during instrumentation, which must include any class that could be loaded from the bootstrap class loader. The former might lose some functionality, and note doing so only fixes the problem for the default options: enabling inclbootstrapclasses=true or inclextclasses=true would still risk a deadlock. The latter is challenging (perhaps use ASM to inspect JaCoCo classes at build time to create a complete list the agent should preload?) and not good for startup performance, but it would only affect inclbootstrapclasses=true and inclextclasses=true JVMs that are likely slow anyway. |
|
@bjkail Thanks for the detailed explanation! Do you think you could create a minimal reproducer? JaCoCo agent is already minimized. So preloading all classes would actually be an option if this solves deadlocks. But would only do this if can proof with a reproducer. |
|
@bjkail Yeah, As usual - thank you very much for your valuable comments. Just some thoughts, guys, about reproducer: So that seems that to trigger loading of classes on a line with logger, we should trigger exception during instrumentation. Or in other cases trigger loading of classes by ASM - I do remember that there are some conditions, which cause ASM to do this, but don't remember exactly which ones. |
|
I don't know how to write a reliable reproducer without modifying the JaCoCo agent to pause to get another thread stuck. However, while investigating to see if I could write a reproducer, I remembered that this problem is mostly solved as of Java 7 due to AppClassLoader and ExtClassLoader being registered as parallel class loaders (my ClassLoader expertise mostly came from Java 6 and earlier era :-)). Does JaCoCo still support Java 6 and earlier? If yes, perhaps add the inclextclasses option and recommend that users enable that in older releases to avoid the deadlock. If no, then perhaps just close this issue. |
|
@Godin The infamous scenario where ASM loads classes unexpectedly is ClassWriter.getCommonSuperclass when COMPUTE_FRAMES is enabled. Perhaps that's what you're remembering. |
|
@bjkail as of today we support quite a huge range - from dinosaur JDK 5 up to bleeding edge JDK 9 EA 😉 As this is about race condition, reproducer with sleep in agent will be a really good start. Then it might be possible to do the same operations in a tight loop or to clock down CPU or slow down JDK to try to demonstrate the same without sleep. So could you please share it? Given the facts that this ticket was created in Dec 2012 against JaCoCo version <= 0.6.0, most likely problem was faced with Java 6. Not sure whether it worth it or not, but could be possible to determine more precisely using JDK parts of stack trace as a breadcrumbs 😈 Regarding parallel class loaders and Java 7: mentioned earlier https://bugs.openjdk.java.net/browse/JDK-8140477 registered as affecting JDK 9 and with comment that problem comes exactly from parallel capability of class loaders (not sure if it means that earlier versions don't suffer from the same problem). And you're probably right about my memory regarding unexpected loading of classes by ASM. |
|
@marchof wow, wasn't aware that they worked out fix. I can't find branch, or you meant patch attached to http://forge.ow2.org/tracker/?func=detail&aid=317551&group_id=23&atid=100023 ? Can try to give you a hand in testing it. |
|
I can reproduce the issue reliably on Java 6 with jacoco54.zip, and it passes on Java 8 as expected. Rather than adding a sleep, I "faked" the main thread with a manual synchronized+defineClass, but hopefully this is still useful to demonstrate the problem. Yes, JDK-8140477 is the same problem as this issue. In JDK 7/8, it was sun.misc.Launcher$AppClassLoader and $ExtClassLoader (see stack traces above), but in JDK 9 those classes were moved/rewritten as part of JEP 261 to jdk.internal.misc.ClassLoaders$AppClassLoader and $ExtClassLoader, so I guess they regressed and then fixed the issue by registering the new class loader implementations as parallel capable. |
|
@bjkail I didn't yet looked inside, so maybe I'm doing something wrong, but seems that it doesn't work for me: |
|
@bjkail my bad, should have read your comments inside perfectly reproduces the problem for me 👍 Thank you! |
|
@Godin I should have given clearer instructions, sorry. jacoco54.Main needs to be excluded because the test requires that synchronized blocks are set up before the JaCoCo code runs for the first time and loads its dependency classes. Glad you found my comment and got it working. |
|
As was pointed out by @bjkail - set of classes for preloading should include not only our classes, but also JVM classes reachable from ours, so that it can't be precomputed at build time and should be computed at runtime. I tried to implement computation of this set using ASM followed by their preloading - see attached commits. There are two versions even. But they anyway fail on a given example: For the time being I did not dig deeper why this happens. |
|
Preloader is closer to what I imagined than Preloader2 since you need to be sure that no code path in JaCoCo will attempt to load a class. If you want to keep digging, I suppose the challenge is determining which class didn't get preloaded? Perhaps |
|
@bjkail I completely forgot about |
|
Ah, ok. I did not look closely enough, but if ClassRemapper can be used to simplify finding the dependencies, then that seems reasonable. |
|
Weird, but in |
We got dead lock on class loading.
In our case appClassLoader is excluded. But I see, that jacoco tries to instrument class that is loaded by appClassLoader. How can i workaround this situation
junit launcher:
<jacoco:coverage enabled="@{test.coverage}" destfile="${coverage.File}" includes="com.xxx.tool.:com.xxx.ci.:com.nomagic.cts.:com.xxx.common."
excludes="org.:java.:com.nomagic.rcpf.:com.xxx.registration.:sun.:javax.:com.oracle.:com.sun." exclclassloader="sun.misc.Launcher$AppClassLoader:sun.reflect.DelegatingClassLoader">
Thread dump
Found one Java-level deadlock: