RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]
Jaikiran Pai
jpai at openjdk.java.net
Tue Mar 16 02:37:24 UTC 2021
On Mon, 15 Mar 2021 17:15:59 GMT, Chris Hegarty <chegar at openjdk.org> wrote:
>>> What you have is probably fine, but has any consideration been given to using the initialization-on-demand holder idiom? e.g.
>>>
>>> ```
>>> static private class CanonicalMapHolder {
>>> static final Map<MethodHandleDesc, Function<DynamicConstantDesc<?>, ConstantDesc>> CANONICAL_MAP
>>> = Map.ofEntries(..);
>>> }
>>> ```
>>
>> Hello Chris,
>>
>> Although I had thought of some other alternate ways to fix this, this idiom isn't something that I had thought of. Now that you showed this, I thought about it a bit more and it does look a lot more "natural" to read and maintain as compared to the current changes in this PR which does some juggling to avoid this deadlock. However, having thought about your proposed solution, I think _in theory_ it still leaves open the possibility of a deadlock.
>>
>> Here's what the patch looks like with your suggested change:
>>
>> diff --git a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> index 976b09e5665..c7bdcf4ce32 100644
>> --- a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> +++ b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
>> @@ -64,8 +64,6 @@ public abstract class DynamicConstantDesc<T>
>> private final String constantName;
>> private final ClassDesc constantType;
>>
>> - private static Map<MethodHandleDesc, Function<DynamicConstantDesc<?>, ConstantDesc>> canonicalMap;
>> -
>> /**
>> * Creates a nominal descriptor for a dynamic constant.
>> *
>> @@ -274,25 +272,7 @@ public abstract class DynamicConstantDesc<T>
>> }
>>
>> private ConstantDesc tryCanonicalize() {
>> - var canonDescs = canonicalMap;
>> - if (canonDescs == null) {
>> - canonDescs = Map.ofEntries(
>> - Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS, DynamicConstantDesc::canonicalizePrimitiveClass),
>> - Map.entry(ConstantDescs.BSM_ENUM_CONSTANT, DynamicConstantDesc::canonicalizeEnum),
>> - Map.entry(ConstantDescs.BSM_NULL_CONSTANT, DynamicConstantDesc::canonicalizeNull),
>> - Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD, DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
>> - Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD, DynamicConstantDesc::canonicalizeFieldVarHandle),
>> - Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY, DynamicConstantDesc::canonicalizeArrayVarHandle));
>> - synchronized (DynamicConstantDesc.class) {
>> - if (canonicalMap == null) {
>> - canonicalMap = canonDescs;
>> - } else {
>> - canonDescs = canonicalMap;
>> - }
>> - }
>> - }
>> -
>> - Function<DynamicConstantDesc<?>, ConstantDesc> f = canonDescs.get(bootstrapMethod);
>> + Function<DynamicConstantDesc<?>, ConstantDesc> f = CanonicalMapHolder.CANONICAL_MAP.get(bootstrapMethod);
>> if (f != null) {
>> try {
>> return f.apply(this);
>> @@ -405,4 +385,15 @@ public abstract class DynamicConstantDesc<T>
>> super(bootstrapMethod, constantName, constantType, bootstrapArgs);
>> }
>> }
>> +
>> + private static final class CanonicalMapHolder {
>> + static final Map<MethodHandleDesc, Function<DynamicConstantDesc<?>, ConstantDesc>> CANONICAL_MAP =
>> + Map.ofEntries(
>> + Map.entry(ConstantDescs.BSM_PRIMITIVE_CLASS, DynamicConstantDesc::canonicalizePrimitiveClass),
>> + Map.entry(ConstantDescs.BSM_ENUM_CONSTANT, DynamicConstantDesc::canonicalizeEnum),
>> + Map.entry(ConstantDescs.BSM_NULL_CONSTANT, DynamicConstantDesc::canonicalizeNull),
>> + Map.entry(ConstantDescs.BSM_VARHANDLE_STATIC_FIELD, DynamicConstantDesc::canonicalizeStaticFieldVarHandle),
>> + Map.entry(ConstantDescs.BSM_VARHANDLE_FIELD, DynamicConstantDesc::canonicalizeFieldVarHandle),
>> + Map.entry(ConstantDescs.BSM_VARHANDLE_ARRAY, DynamicConstantDesc::canonicalizeArrayVarHandle));
>> + }
>> }
>>
>>
>> Please consider the following, where I try and explain the theoretical possibility of a deadlock with this approach:
>>
>> 1. Let's consider 2 threads T1 and T2 doing concurrent execution
>>
>> 2. Let's for a moment assume that neither `DynamicConstantDesc` nor `ConstantDescs` classes have been initialized.
>>
>> 3. T1 does a call to `DynamicConstantDesc.ofCanonical(...)` and T2 does a call to something/anything on `ConstantDescs`, which triggers a class initialization of `ConstantDescs`.
>>
>> 4. T1 (which called `DynamicConstantDesc.ofCanonical(...)`) will reach the `tryCanonicalize` and will access `CanonicalMapHolder.CANONICAL_MAP` in the implementation of that method. Because of this access (and since `CanonicalMapHolder` hasn't yet been initialized), T1 will obtain (an implicit) lock on the `CanonicalMapHolder` class (for the class initialization). Let's assume T1 is granted this lock on `CanonicalMapHolder` class and it goes ahead into the static block of that holder class to do the initialization of `CANONICAL_MAP`. To do so, because of the code in the static block of `CanonicalMapHolder`, it now requires a class initialization lock on `ConstantDescs` (since `ConstantDescs` hasn't yet been initialized). So it requests for that lock on `ConstantDescs` class.
>>
>> 5. Concurrently, let's say T2 has initiated a class initialization of `ConstantDescs`. So T2 is currently holding a class initialization lock on `ConstantDescs`. While the static initialization of `ConstantDescs` is going on, let's assume (in theory), due to the whole lot of code in the static initialization of `ConstantDescs`, it either directly or indirectly ends up calling `DynamicConstantDesc.ofCanonical(...)`. This then means that T2 now enters the `tryCanonicalize` and accesses `CanonicalMapHolder.CANONICAL_MAP` and since `CanonicalMapHolder` hasn't yet been (fully) initialized (remember T1 is still in the static block of `CanonicalMapHolder`), it tries to obtain a class initialization lock on `CanonicalMapHolder` which is currently held by T1. T1 won't let go that lock since it wants the lock on `ConstantDescs` which is currently held by T2. This effectively ends up triggering the deadlock.
>>
>> This deadlock isn't possible with the current approach that this PR has.
>>
>> I want to clarify that, the deadlock that I explain above with your proposed approach is just a theoretical possibility. The `ConstantDescs` doesn't currently have any direct call to `DynamicConstantDesc.ofCanonical` in its static initialization nor can I see or think of any indirect calls to `DynamicConstantDesc.ofCanonical` from its static block. I need input from you whether I should keep aside this theoretic issue and deal with it if/when we run into it (the test case in this PR, IMO, is robust enough to catch that deadlock if/when it happens due to any future code changes in this area). If you and others think that we can ignore this case, then your proposed approach of using this lazy holder for initialization, IMO, is much cleaner and natural to read and I will go ahead and update this PR to use it.
>>
>> For those interested in seeing this potential theoretical deadlock with the lazy holder approach in action, I just created a separate branch here https://github.com/jaikiran/jdk/commits/8263108-demo-deadlock-theory which consistently reproduces the deadlock when the `DynamicConstantDescTest` jtreg test is run. The crucial part is that I introduced the following (dummy) call in `ConstantDescs`:
>>
>> diff --git a/src/java.base/share/classes/java/lang/constant/ConstantDescs.java b/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> index 3a000d1bd4a..3f793f5a8b3 100644
>> --- a/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> +++ b/src/java.base/share/classes/java/lang/constant/ConstantDescs.java
>> @@ -286,6 +286,13 @@ public final class ConstantDescs {
>> static final DirectMethodHandleDesc MHD_METHODHANDLE_ASTYPE
>> = MethodHandleDesc.ofMethod(Kind.VIRTUAL, CD_MethodHandle, "asType",
>> MethodTypeDesc.of(CD_MethodHandle, CD_MethodType));
>> +
>> + static {
>> + // just a dummy call to DynamicConstantDesc.ofCanonical
>> + DynamicConstantDesc.ofCanonical(ConstantDescs.BSM_ENUM_CONSTANT, "SHOW_REFLECT_FRAMES",
>> + ClassDesc.of("java.lang.StackWalker").nested("Option"), new ConstantDesc[0]);
>> + }
>> +
>>
>>
>> The following thread dump shows the deadlock with that lazy holder approach:
>>
>> "MainThread" #15 prio=5 os_prio=31 cpu=6.26ms elapsed=120.18s tid=0x00007fe0e58df800 nid=0x5e03 waiting on condition [0x000070000d71d000]
>> java.lang.Thread.State: WAITING (parking)
>> at jdk.internal.misc.Unsafe.park(java.base at 17-internal/Native Method)
>> - parking to wait for <0x000000070ff65d90> (a java.util.concurrent.FutureTask)
>> at java.util.concurrent.locks.LockSupport.park(java.base at 17-internal/LockSupport.java:211)
>> at java.util.concurrent.FutureTask.awaitDone(java.base at 17-internal/FutureTask.java:447)
>> at java.util.concurrent.FutureTask.get(java.base at 17-internal/FutureTask.java:190)
>> at DynamicConstantDescTest.main(DynamicConstantDescTest.java:80)
>> at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base at 17-internal/Native Method)
>> at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base at 17-internal/NativeMethodAccessorImpl.java:78)
>> at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base at 17-internal/DelegatingMethodAccessorImpl.java:43)
>> at java.lang.reflect.Method.invoke(java.base at 17-internal/Method.java:566)
>> at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
>> at java.lang.Thread.run(java.base at 17-internal/Thread.java:831)
>>
>> "pool-1-thread-1" #16 prio=5 os_prio=31 cpu=14.32ms elapsed=120.18s tid=0x00007fe0e58dea00 nid=0x9903 waiting on condition [0x000070000d820000]
>> java.lang.Thread.State: WAITING (parking)
>> at jdk.internal.misc.Unsafe.park(java.base at 17-internal/Native Method)
>> - parking to wait for <0x000000070ff59d30> (a java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
>> at java.util.concurrent.locks.LockSupport.park(java.base at 17-internal/LockSupport.java:341)
>> at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode.block(java.base at 17-internal/AbstractQueuedSynchronizer.java:506)
>> at java.util.concurrent.ForkJoinPool.unmanagedBlock(java.base at 17-internal/ForkJoinPool.java:3454)
>> at java.util.concurrent.ForkJoinPool.managedBlock(java.base at 17-internal/ForkJoinPool.java:3425)
>> at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(java.base at 17-internal/AbstractQueuedSynchronizer.java:1623)
>> at java.util.concurrent.LinkedBlockingQueue.take(java.base at 17-internal/LinkedBlockingQueue.java:435)
>> at java.util.concurrent.ThreadPoolExecutor.getTask(java.base at 17-internal/ThreadPoolExecutor.java:1061)
>> at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base at 17-internal/ThreadPoolExecutor.java:1121)
>> at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base at 17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base at 17-internal/Thread.java:831)
>>
>> "pool-1-thread-2" #17 prio=5 os_prio=31 cpu=7.88ms elapsed=120.18s tid=0x00007fe0e4012c00 nid=0x6003 in Object.wait() [0x000070000d923000]
>> java.lang.Thread.State: RUNNABLE
>> at java.lang.constant.DynamicConstantDesc.tryCanonicalize(java.base at 17-internal/DynamicConstantDesc.java:275)
>> - waiting on the Class initialization monitor for java.lang.constant.DynamicConstantDesc$CanonicalMapHolder
>> at java.lang.constant.DynamicConstantDesc.ofCanonical(java.base at 17-internal/DynamicConstantDesc.java:136)
>> at DynamicConstantDescTest$InvokeOfCanonical.call(DynamicConstantDescTest.java:129)
>> at java.util.concurrent.FutureTask.run(java.base at 17-internal/FutureTask.java:264)
>> at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base at 17-internal/ThreadPoolExecutor.java:1135)
>> at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base at 17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base at 17-internal/Thread.java:831)
>>
>> "pool-1-thread-3" #18 prio=5 os_prio=31 cpu=15.43ms elapsed=120.18s tid=0x00007fe0e4070600 nid=0x9803 in Object.wait() [0x000070000da26000]
>> java.lang.Thread.State: RUNNABLE
>> at java.lang.constant.DynamicConstantDesc.tryCanonicalize(java.base at 17-internal/DynamicConstantDesc.java:275)
>> - waiting on the Class initialization monitor for java.lang.constant.DynamicConstantDesc$CanonicalMapHolder
>> at java.lang.constant.DynamicConstantDesc.ofCanonical(java.base at 17-internal/DynamicConstantDesc.java:136)
>> at java.lang.constant.ConstantDescs.<clinit>(java.base at 17-internal/ConstantDescs.java:292)
>> at java.lang.Class.forName0(java.base at 17-internal/Native Method)
>> at java.lang.Class.forName(java.base at 17-internal/Class.java:375)
>> at DynamicConstantDescTest$Task.call(DynamicConstantDescTest.java:104)
>> at DynamicConstantDescTest$Task.call(DynamicConstantDescTest.java:87)
>> at java.util.concurrent.FutureTask.run(java.base at 17-internal/FutureTask.java:264)
>> at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base at 17-internal/ThreadPoolExecutor.java:1135)
>> at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base at 17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base at 17-internal/Thread.java:831)
>>
>> "pool-1-thread-4" #19 prio=5 os_prio=31 cpu=5.39ms elapsed=120.18s tid=0x00007fe0e4070c00 nid=0x9603 in Object.wait() [0x000070000db29000]
>> java.lang.Thread.State: RUNNABLE
>> at java.lang.constant.DynamicConstantDesc$CanonicalMapHolder.<clinit>(java.base at 17-internal/DynamicConstantDesc.java:390)
>> - waiting on the Class initialization monitor for java.lang.constant.ConstantDescs
>> at java.lang.constant.DynamicConstantDesc.tryCanonicalize(java.base at 17-internal/DynamicConstantDesc.java:275)
>> at java.lang.constant.DynamicConstantDesc.ofCanonical(java.base at 17-internal/DynamicConstantDesc.java:136)
>> at DynamicConstantDescTest$InvokeOfCanonical.call(DynamicConstantDescTest.java:129)
>> at java.util.concurrent.FutureTask.run(java.base at 17-internal/FutureTask.java:264)
>> at java.util.concurrent.ThreadPoolExecutor.runWorker(java.base at 17-internal/ThreadPoolExecutor.java:1135)
>> at java.util.concurrent.ThreadPoolExecutor$Worker.run(java.base at 17-internal/ThreadPoolExecutor.java:635)
>> at java.lang.Thread.run(java.base at 17-internal/Thread.java:831)
>
>> If you and others think that we can ignore this case, then your proposed approach of using this lazy holder for initialization, IMO, is much cleaner and natural to read and I will go ahead and update this PR to use it.
>
> For me, at least, the holder pattern is clearer. I'm happy with that approach. ( I don't have an objection to the alternative, just a mild preference for the holder )
Hello Chris, using the holder pattern sounds fine to me then. I've updated this PR accordingly. The test continues to pass with this fix.
Peter, I didn't get a chance to try out the `@Stable` for the previous approach but given that we decided to use this holder pattern, we will no longer need the performance tweaks.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2893
More information about the core-libs-dev
mailing list