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

[Anton Artemov]

On Fri, 14 Nov 2025 18:07:27 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 22 commits:
>> 
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Fixed the test.
>>  - 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
>>  - ... and 12 more: https://git.openjdk.org/jdk/compare/ff851de8...2ac18b94
>
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 124:
> 
>> 122: // launch resumer       enter threadLock
>> 123: // <launch returns>     while !READY_TO_NOTIFY  resumer running
>> 124: // delay 1-second         threadLock.wait(1)    wait for notify
> 
> We've deleted this sleep, so this line should change from:
> 
> // delay 1-second         threadLock.wait(1)    wait for notify
> 
> to:
> 
> // :                      threadLock.wait(1)    wait for notify

Addressed.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 128:
> 
>> 126: // set READY_TO_NOTIFY  :
>> 127: // threadLock.notify    wait finishes           :
>> 128: // :                    reenter blocks          :
> 
> We added a sleep here, so this line should change from:
> 
> // :                    reenter blocks          :
> 
> to:
> 
> // : delay 200ms        reenter blocks          :

Addressed.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 153:
> 
>> 151: //       thread allows the waiter thread to loop tightly here:
>> 152: //         while !READY_TO_NOTIFY
>> 153: //           threadLock.wait(1)
> 
> Please replace this note with the following:
> 
> // Note: sleep(200ms) here while holding the threadLock to allow the waiter thread's
> // timed wait to finish before we attempt to suspend the waiter thread.

Addressed.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 237:
> 
>> 235: 
>> 236:     public static void usage() {
>> 237:         System.err.println("Usage: " + AGENT_LIB + " [N][-p][time_max]");
> 
> The `N` argument is not optional so it should not be surrounded by `[` and `]`.

Addressed in the latest commit.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 239:
> 
>> 237:         System.err.println("Usage: " + AGENT_LIB + " [N][-p][time_max]");
>> 238:         System.err.println("where:");
>> 239:         System.err.println("    N        ::= test case");
> 
> s/case/case: 1 | 2 | 3/

Addressed in the latest commit.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 470:
> 
>> 468:                 }
>> 469:                 try {
>> 470:                     Thread.sleep(1000);
> 
> Please add something like this above the `sleep`:
> 
> // Delay for 1-second while holding the threadLock to force the
> // resumer thread to block on entering the threadLock.

Addressed in the latest commit.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 552:
> 
>> 550:                 // wait for the waiter thread to block
>> 551:                 logDebug("before contended enter wait");
>> 552:                 int retCode = wait4ContendedEnter(waiter);
> 
> Since we are waiting here for ContendedEnter by the waiter thread, do we really
> need the above `sleep(200)` or are we just being polite CPU resource wise?

Maybe I do not fully understand the question, but isn't it what differentiates test case 2 from test case 3?

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait.java line 592:
> 
>> 590:                 }
>> 591:                 try {
>> 592:                     Thread.sleep(1000);
> 
> Please add something like this above the sleep:
> 
> // Delay for 1-second while holding the threadLock to force the
> // resumer thread to block on entering the threadLock.

Added.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534136973
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534137344
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534137633
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534135509
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534135744
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534138176
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534136258
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2534136725