On Tue, 18 Feb 2025 02:58:22 GMT, Chris Plummer <[email protected]> wrote:
>> src/jdk.jdwp.agent/share/native/libjdwp/stepControl.c line 899:
>>
>>> 897: }
>>> 898:
>>> 899: if (step->pending) {
>>
>> Is this condition the same as new line 876 above? (I don't see where the
>> intervening calls would affect it?)
>
> That's a good question. I hadn't noticed the extra check for step->pending,
> and it looks like that was also in place for the previous version that relied
> on calling ClearFramePop on each outstanding NotifyFramePop. I think it is
> just bit rot as the code initially evolved. If it was possibly for the flag
> to be cleared by some other thread (I actually think it might be, but need to
> look closer), this extra check is not of much help since it could be cleared
> after the check. Let me look into this a bit more.
There are multiple threads that can end up calling clearStep(). However, all
callers of clearStep() grab the eventHandler_lock() first, with one somewhat
obscure exception:
debugInit_reset
threadControl_reset
removeVThreads
clearThread
stepControl_clearRequest
clearStep
This is done for a debugger reset after a connection has been dropped. Probably
not at much risk of a race in clearStep, but I think is best to add
eventHandler locking here also. After doing this there will be no race
conditions to worry about.
I'm pretty sure this extra check for "pending" is just bit rot. I know I wanted
an if block around the new code to make it easy to disable so I could easily
benchmark and test with and without "the fix". I think at one point the code
looked a lot different (I think there was something outside of the if), but
that was long ago.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23182#discussion_r1960478633
