On Wed, 20 Oct 2021 06:50:51 GMT, Richard Reingruber <rr...@openjdk.org> wrote:
>> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 770:
>>
>>> 768: * handlerLock which has to be acquired before threadLock.
>>> 769: */
>>> 770: debugMonitorExit(threadLock);
>>
>> I think we can avoid the exit here. threadLock was grabbed in
>> `threadControl_onEventHandlerExit()`. It probably should be released before
>> calling `doPendingTasks()`. My suggestion would be to first release it right
>> after the `findThread()` call, and then in the `ei == EI_THREAD_END` block
>> grab it again at the start of the block and release at the end of the block.
>
> But fields of ThreadNode "node" (aka "resumer") are read and also modified in
> `doPendingTasks()` and also in `threadControl_onEventHandlerExit()`. IMHO
> this needs to be synchronized with threadLock. At least it is not obvious
> that the synchronization is not needed.
You already release threadLock temporarily in `trackAppResume()` and you used
to release it temporarily in doPendingTasks(). I'm suggesting to instead
release it in `threadControl_onEventHandlerExit()` before calling
`doPendingTasks()` and then grab it again (after first grabbing the
handlerLock) in `doPendingTasks()` before calling `trackAppResume()`. What can
be modified in the ThreadNode during that time that you are concerned about?
threadLock would still be held when `blockOnDebuggerSuspend()` is called. The
only interesting lines that are currently executed while holding the threadLock
but no longer would be are:
2193 if (node->handlingAppResume) {
2194 jthread resumer = node->thread;
2195 jthread resumee = getResumee(resumer);
I don't think any of those can change, because only the currently executing
thread can change them (when the Thread.resume() breakpoint is hit).
The suspendCount is only meaningful when in `blockOnDebuggerSuspend()`, and
we'll be under the threadLock by that point.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5849
