On Wed, 16 Feb 2022 00:16:48 GMT, Zhengyu Gu <[email protected]> wrote:
>> There are scenarios that JDWP agent can deadlock on `classTrackLock`
>> monitor. Following is the scenario in bug report.
>>
>> **Java Thread**
>> - loads a class and post `JVMTI_EVENT_CLASS_PREPARE` event
>> - JDWP event callback handler calls `classTrack_processUnloads()` to handle
>> the event.
>> - `classTrack_processUnloads()` takes `classTrackLock` lock, then tries to
>> allocate a new bag under the lock.
>> - bag allocation code calls` jvmtiAllocate()`, which may be blocked by
>> ongoing safepoint due to state transition.
>>
>> If the safepoint is GC safepoint (prior to JDK16) or
>> `VM_JvmtiPostObjectFree` safepoint (JDK16 or later)
>>
>> **VM Thread**
>> - post `JVMTI_EVENT_OBJECT_FREE`
>> - JDWP event callback handler calls `cbTrackingObjectFree()` to handle the
>> event
>> - `cbTrackingObjectFree()` tries to acquire `classTrackLock` lock, leads to
>> deadlock
>>
>> From my research, there are three events that may be posted at safepoints,
>> `JVMTI_EVENT_GARBAGE_COLLECTION_START`,
>> `JVMTI_EVENT_GARBAGE_COLLECTION_FINISH` and `JVMTI_EVENT_OBJECT_FREE`, but
>> only `JVMTI_EVENT_OBJECT_FREE` is relevant to JDWP agent.
>>
>> The solution I purpose here, is simply move allocation/deallocation code
>> outside of `classTrackLock` lock.
>>
>>
>> Test:
>> - [x] tier1
>> - [x] vmTestbase_nsk_jdi
>> - [x] vmTestbase_nsk_jdwp
>> - [x] vmTestbase_nsk_jvmti
>
> Zhengyu Gu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Simplify classTrack_reset and revert bagSize check
processUnloads is continuously called on every event. It's purpose is to
process pending CLASS_UNLOAD events that have accumulated since the last JVMTI
event. This is a rather hacky solution. JVMTI does not actually send a
CLASS_UNLOAD event. It doesn't support them. What it does support is
OBJECT_FREE events. So when the debugger has enabled CLASS_UNLOAD events, the
debug agent enables OBJECT_FEE events. It tags each j.l.Class instance so it
will get the OBJECT_FREE event for them, and when the event comes in, the Class
gets added to deletedSignatures so the agent can later notify the debugger
about it.
As a side note, this OBJECT_FREE solution is somewhat new. The old solution was
to always do an "allClasses" during initialization, and then diff that with
"allClasses" every time there was a full GC to see which classes unloaded. I
wonder now if there isn't a reason we just don't send the CLASS_UNLOAD when the
OBJECT_FREE is received. It would probably greatly simplify classTrack.c, and
will fix an outstanding bug we have where sometimes there is a lengthy delay
before CLASS_UNLOAD events are sent. This is because once there is a GC, they
events are not sent until the next JVMTI event. It's actually possible there
would never be one if the only thing the debugger is requesting is CLASS_UNLOAD
events.
Anyway, back to processUnloads synchronization. The reason for synchronization
between processUnload and activate is because one thread could be handling some
random JVMTI event (maybe a breakpoint) and is calling processUnloads, and at
the same time another thread is handling a CLASS_UNLOAD event request from the
debugger, and calling activate. But now that I think of it, this should be
harmless even without synchronization. The processUnloads thread will see the
allocated deletedSignatures if activate has gotten that far, otherwise it
won't. It should behave correctly in either case. I'm not convince that
activate even needs synchronization. The purpose of the classTrack
synchronization is to prevent one thread in processUnloads from grabbing and
using deletedSignatures, only to have another thread in processUnloads do the
same, and delete deletedSignatures in the process, leaving the 1st thread
holding onto a deallocated pointer. However, even this should not be possible
because a
ll callers to processUnloads have already grabbed the handlerLock.
What I'm tempted to say here is just have this PR keep all the synchronization
and I'll file a CR to possibly do a lot of cleanup here. Either remove (maybe
all of) the synchronization, or maybe even implement sending the CLASS_UNLOAD
when the OBJECT_FREE comes in, which will make the synchronization question
moot, and also fix that other bug I mentioned.
BTW, one other bit of nonsense. classTrack_activate() does:
deletedSignatures = bagCreateBag(sizeof(char*), 1000);
This will be deleted the very first time an event comes in, even if no classes
were unloaded, and processUnloads replaces it with:
deletedSignatures = bagCreateBag(sizeof(char*), 10);
-------------
PR: https://git.openjdk.java.net/jdk/pull/7461
