RFR: 8366659: ObjectMonitor::wait() liveness problem with a suspension request [v20]

[Anton Artemov]

On Mon, 19 Jan 2026 00:27:39 GMT, David Holmes <dholmes 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 44 commits:
>> 
>>  - 8366659: Fixed year in the copyright.
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Removed ClearSuccOnSuspend
>>  - 8366659: Fixed liveness problem.
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Fixed build problem.
>>  - 8366659: Fixed build issue.
>>  - 8366659: Changed the way how notify_internal works if JVMTI monitor waited event allowed.
>>  - Merge remote-tracking branch 'origin/master' into JDK-8366659-OM-wait-suspend-deadlock
>>  - 8366659: Fixed semi-broken tests
>>  - ... and 34 more: https://git.openjdk.org/jdk/compare/a01283a5...21b83214
>
> src/hotspot/share/runtime/objectMonitor.cpp line 1907:
> 
>> 1905:     // That is, we fail toward safety.
>> 1906: 
>> 1907:     was_notified = true;
> 
> This isn't quite right. If we would reach line 1882 then we were not notified, so we cannot unconditionally set this to true here. I think you need to initialize to true at line 1868 and then set to false at line 1882 (deleting the current comment).

Thanks for spotting this! Addressed.

> src/hotspot/share/runtime/objectMonitor.cpp line 1917:
> 
>> 1915:     }
>> 1916: 
>> 1917:     // The thread is now either on off-list (TS_RUN),
> 
> Suggestion:
> 
>     // The thread is now either off-list (TS_RUN),

Addressed.

> src/hotspot/share/runtime/objectMonitor.cpp line 1932:
> 
>> 1930:     // (Don't cache naked oops over safepoints, of course).
>> 1931: 
>> 1932:     // post monitor waited event. Note that this is past-tense, we are done waiting.
> 
> Suggestion:
> 
>     // Post monitor waited event. Note that this is past-tense, we are done waiting.
> 
> You need to expand the comment to explain the significance of checking for TS_ENTER.

Addressed.

> src/hotspot/share/runtime/objectMonitor.cpp line 1973:
> 
>> 1971:         // and set the OM as pending.
>> 1972:       }
>> 1973:       // Done waiting, post the corresponding event
> 
> What event? I expected to see `JvmtiExport::post_monitor_waited` here. ??

I think I overlooked this comment, it is related to one of the previous iterations, where the `JvmtiExport::post_monitor_waited` event was posted further down in the code. Removed.

> src/hotspot/share/runtime/objectMonitor.cpp line 2053:
> 
>> 2051:       // Adding to _entry_list uses Atomic::cmpxchg() which already provides
>> 2052:       // a fence that prevents reordering of the stores.
>> 2053:       if (!JvmtiExport::should_post_monitor_waited()) {
> 
> You need to explain why this is now conditional.

Added a comment there.

> src/hotspot/share/runtime/objectMonitor.cpp line 2063:
> 
>> 2061:     if (!JvmtiExport::should_post_monitor_waited()) {
>> 2062:       add_to_entry_list(current, iterator);
>> 2063:     } else {
> 
> You need to explain what each of these paths is doing. At this point I'm lost.

Added extended comments for both cases.

> src/hotspot/share/runtime/objectMonitor.cpp line 2069:
> 
>> 2067: 
>> 2068:       oop vthread = nullptr;
>> 2069:       ParkEvent* Trigger;
> 
> I assume this is copied from elsewhere but we should use proper style rules in the new code i.e. local variables don't start with a capital. Thanks. Also "trigger" doesn't really convet anythng meaningful to me - `evt` for event seems fine.

Right, it was copied from some other file. Fixed.

> src/hotspot/share/runtime/objectMonitor.cpp line 2277:
> 
>> 2275:   node->_at_reenter = true;
>> 2276: 
>> 2277:   if (state == ObjectWaiter::TS_RUN) {
> 
> I'm finding the changes to `was_notified` versus use of explicit state checks very confusing. I keep asking myself which state means I was notified and which does not.

Yes, it is all because unparking inside of `notify_internal()`, which is needed to fix the problem, leads to threads which have been actually notified (aka woken up in `notify_internal()'), but are not on the `entry_list`. We may need to do a follow-up PR with some refactoring, maybe binary state is not sufficient.

> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorWait/SuspendWithObjectMonitorWait1.java line 71:
> 
>> 69:     public int doWork1(int timeMax, PrintStream out) {
>> 70:         SuspendWithObjectMonitorWaitWorker waiter;    // waiter thread
>> 71:         SuspendWithObjectMonitorWaitWorker resumer;    // resumer thread
> 
> Suggestion:
> 
>         SuspendWithObjectMonitorWaitWorker waiter;    // waiter thread
>         SuspendWithObjectMonitorWaitWorker resumer;   // resumer thread

Fixed in all test cases.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704328783
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704329246
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704329583
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330248
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330507
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330740
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704330960
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704331400
PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2704331637