On Tue, 19 Oct 2021 20:17:59 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:

>> Richard Reingruber has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Call blockOnDebuggerSuspend() after setup of the resumer tracking.
>
> 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.

> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 771:
> 
>> 769:      */
>> 770:     debugMonitorExit(threadLock);
>> 771:     eventHandler_lock(); /* for proper lock order */
> 
> "for proper lock order during eventHandler_createInternalThreadOnly() calls"

Done.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5849

Reply via email to