On Thu, 21 Oct 2021 03:24:40 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> 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.
BTW, normally I would not suggest less locking simply because I *thought* it
might not be needed. However, you already temporarily release and then regrab
the lock. This means there is a period of time where the locking is not needed.
What I've suggested is to better define (and expand) that period of time so the
locking can be made cleaner. I think the actual expansion of time where the
lock would no longer be held is pretty small, with a well defined set of code
being executed that to me does not appear to require the lock, and if it did
require the lock, then probably releasing the lock in the first place is a bug.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5849
