On Fri, 23 Jan 2026 09:05:58 GMT, Anton Artemov <[email protected]> wrote:
>> Hi, please consider the following changes: >> >> If suspension is allowed when a thread is re-entering an object monitor >> (OM), then a following liveness issues can happen in the >> `ObjectMonitor::wait()` method. >> >> The waiting thread is made to be a successor and is unparked. Upon a >> suspension request, the thread will suspend itself whilst clearing the >> successor. The OM will be left unlocked (not grabbed by any thread), while >> the other threads are parked until a thread grabs the OM and the exits it. >> The suspended thread is on the entry-list and can be selected as a successor >> again. None of other threads can be woken up to grab the OM until the >> suspended thread has been resumed and successfully releases the OM. >> >> This can happen in three places where the successor could be suspended: >> >> 1: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1897 >> >> 2: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1149 >> >> 3: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1951 >> >> The issues are addressed by not allowing suspension in case 1, and by >> handling the suspension request at a later stage, after the thread has >> grabbed the OM in `reenter_internal()` in case 2. In case of a suspension >> request, the thread exits the OM and enters it again once resumed. >> >> Case 3 is handled by not transferring a thread to the `entry_list` in >> `notify_internal()` in case the corresponding JVMTI event is allowed. >> Instead, a tread is unparked and let run. Since it is not on the >> `entry_list`, it will not be chosen as a successor and it is no harm to >> suspend it if needed when posting the event. >> >> Possible issue of posting a `waited` event while still be suspended is >> addressed by adding a suspension check just before the posting of event. >> >> Tests are added. >> >> Tested in tiers 1 - 7. > > Anton Artemov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 56 commits: > > - Merge remote-tracking branch 'origin/master' into > JDK-8366659-OM-wait-suspend-deadlock > - 8366659: Changed condition on when to post an event. > - 8366659: Fixed whitespaces. > - 8366659: Addressed reviewer's comments. > - 8366659: Addressed reviewer's comments. > - 8366659: Addressed reviewers' comments. > - 8366659: Fixed whitespace. > - 8366659: Addressed reviewer's comments. > - 8366659: Addressed reviewers' comments, added comments, renamed tests. > - 8366659: Modified the comment. > - ... and 46 more: https://git.openjdk.org/jdk/compare/fa20391e...31779cad I've made two complete read-thrus today. Just a couple comments about the changes as they are in v30. src/hotspot/share/runtime/objectMonitor.cpp line 1942: > 1940: } > 1941: > 1942: JvmtiExport::post_monitor_waited(current, this, ret == > OS_TIMEOUT); OK so this effectively polls and handles a pending suspend request. That does not guarantee that another suspend request won't be pending again (and handled in the `JvmtiExport::post_monitor_waited()` call). src/hotspot/share/runtime/objectMonitor.cpp line 2099: > 2097: // move the add-to-entry_list operation, above, outside the > critical section > 2098: // protected by _wait_set_lock. In practice that's not useful. > With the > 2099: // exception of wait() timeouts and interrupts the monitor owner nit: extra space before wait. src/hotspot/share/runtime/objectMonitor.cpp line 2242: > 2240: // once we re-acquire the monitor we know if we need to throw IE or > not. > 2241: ObjectWaiter::TStates state = node->TState; > 2242: assert(was_notified || state == ObjectWaiter::TS_RUN, ""); assert with multiple conditions should output the values so we know exactly what condition failed. ------------- Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3699805855 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2722940869 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2722949992 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2722983763
