RFR: 8366659: ObjectMonitor::wait() can deadlock with a suspension request [v6]

[Patricio Chilano Mateo]

On Wed, 12 Nov 2025 19:07:49 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Anton Artemov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 20 commits:
>> 
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Fixed lines in tests.
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Added a comment to a boolean arg for enter()
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Fixed new lines.
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Removed incorrect assert,
>>  - 8366659: Fixed merge conflict
>>  - ... and 10 more: https://git.openjdk.org/jdk/compare/400a83da...702880c6
>
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 388:
> 
>> 386:                 try {
>> 387:                     Thread.sleep(1000);
>> 388:                 } catch(Exception e) {}
> 
> Why is this 1 second delay needed?

We need to give time for the resumer thread to block on the `threadLock` before we release it. We could reduce the amount of sleep time though.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 428:
> 
>> 426:             // launch the waiter thread
>> 427:             synchronized (barrierLaunch) {
>> 428:                 waiter = new SuspendWithObjectMonitorWaitWorker("waiter", 1);
> 
> This change to `wait` for `1` instead of `0` requires this comment to be updated from:
> 
>                 // TS_READY_TO_NOTIFY is set after the main thread has
>                 // entered threadLock so a spurious wakeup can't get the
>                 // waiter thread out of this threadLock.wait(0) call:
> 
> to:
> 
>                 // TS_READY_TO_NOTIFY is set after the main thread has
>                 // entered threadLock so a spurious wakeup can't get the
>                 // waiter thread out of this threadLock.wait(0) call in
>                 // doWork1 or doWork2. doWork3 passes a one so that the
>                 // wait() can terminate early and block on reentry.
> 
> 
> I'm having trouble seeing why this third test case is necessary. We do a short `wait(1)`
> in this test case instead of a `wait(0)` so we terminate the `wait(1)` with a timeout instead
> of a `notify()` from the main thread. 
> 
> In all worker test cases, the main thread grabs the threadsLock when the "waiter" thread
> calls `wait()`, the main thread does a `notify()`, the main thread waits until the worker
> thread contends on threadsLock and finally the main thread suspends the worker thread.
> The only thing that I see that the `wait(1)` brings to the party is that the worker3 thread
> might get to re-entry block on threadsLock via a timeout instead of a notify.
> 
> What am I missing here?

The `notify()` call doesn’t unpark the waiter thread, it just moves it from the `_wait_set` to the `_entry_list`. The wait(timeout) is there to allow it to run again so that it is suspended in `reenter_internal()`. But currently the timings will not exercise this case well. I added some comments to fix it.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 454:
> 
>> 452:             try {
>> 453:                 Thread.sleep(1000);
>> 454:             } catch(Exception e) {}
> 
> Why is this 1 second delay needed?

This sleep is not needed.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 509:
> 
>> 507:                 try {
>> 508:                     Thread.sleep(1000);
>> 509:                 } catch(Exception e) {}
> 
> Why is this 1 second delay needed?

Same here, we need to give time for the resumer thread to block on the threadLock before we release it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524648214
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524652123
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524641301
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2524648857