On Wed, 22 Apr 2026 22:22:00 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> Anton Artemov has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - 8382088: Addressed reviewer's comments.
>> - 8382088: Addressed reviewer's comments.
>
> src/hotspot/share/runtime/objectMonitor.cpp line 1904:
>
>> 1902: // An event could have been enabled after notification, in this
>> case
>> 1903: // a thread will have TS_ENTER state and posting the event may hit
>> a suspension point.
>> 1904: // From a debugging perspective, it is more important to have no
>> missing events.
>
> This comment was also added in JDK-8366659 but looks contradictory. If the
> monitor waited event is enabled after the thread was notified, then we don’t
> post the event since the thread will be in the `TS_ENTER` state. So it should
> say that it is more important to avoid deadlocks due to suspension. Also,
> this would not be a case of a missing event. What matters is if the event was
> enabled at the time of notification. Whether the waiter checks before or
> after the event is later enabled is racy and an implementation detail.
> Suggestion: Given that this suspension point is needed even without monitor
> waited event enabled, I would replace the comment with something like: “If
> the thread is not on the _entry_list, we need to check for suspension now
> because the re-enter path will not have a suspension point if there is no
> contention. Also, if the monitor waited event needs to be posted (which also
> implies off the _entry_list) we need to check for suspension before posting
> it.”. And we can also move `Post monitor waited event. Note that this is
> past-tense, we are done waiting.` below, where we check for the event.
Modified the comment as suggested.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 36:
>
>> 34: * @test
>> 35: * @bug 8382088
>> 36: * @summary Suspend thread right after it timed out in wait()
>
> Suggestion:
>
> * @summary Check that a thread suspended at a timed wait() call does not
> reacquire the monitor before it is resumed.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 37:
>
>> 35: * @bug 8382088
>> 36: * @summary Suspend thread right after it timed out in wait()
>> 37: * @requires vm.continuations
>
> Not needed.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 69:
>
>> 67: static long sleepInterval = 20;
>> 68: static int maxRetries = 2000;
>> 69: static long waitForTimedWaitingMills = 5000;
>
> These can be moved to the top where `lock` is defined. `sleepInterval` can be
> removed.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 80:
>
>> 78: waitTask task = new waitTask(countDownLatch);
>> 79: Thread.Builder builder = Thread.ofPlatform();
>> 80: Thread targetThread = builder.name("Target
>> Thread").unstarted(task);
>
> Suggestion:
>
> Thread targetThread = Thread.ofPlatform().name("Target
> Thread").unstarted(task);
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 89:
>
>> 87: }
>> 88:
>> 89: Thread.yield();
>
> Not needed.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 95:
>
>> 93: for (int n = 0; n < maxRetries; ++n) {
>> 94:
>> 95: long deadlineNanos = System.nanoTime() +
>> waitForTimedWaitingMills * 1000_000L;
>
> Suggestion:
>
> long deadlineNanos = System.nanoTime() + waitForTimedWaitingMills
> * 1_000_000L;
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 106:
>
>> 104: boolean grabbedMonitor = false;
>> 105:
>> 106: JVMTIUtils.suspendThread(targetThread);
>
> There is a race here where the target could have already exited before we try
> to suspend it. Adding a long `Thread.sleep` call before this statement will
> make the test fail. I see you have added several `isAlive()` checks in other
> places. All those can be removed and this race fixed just by letting the
> target synchronize with the main thread before exiting. You could use another
> `CountDownLatch` to fix it, but it will be simpler to just use one `Phaser`.
> You can use it to synchronize at the start of each loop iteration (simplify
> target to also just loop on `maxRetries`), and once more at the end before
> the target exits.
Thanks for suggestion, I think the test is more laconic with a single phaser
now.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 124:
>
>> 122: usefulRun += 1;
>> 123:
>> 124: if (sleepInterval > 0) {
>
> This conditional is not needed.
Removed.
> test/hotspot/jtreg/serviceability/jvmti/SuspendWithObjectMonitorTimedWait/SuspendWithObjectMonitorTimedWait.java
> line 126:
>
>> 124: if (sleepInterval > 0) {
>> 125: try {
>> 126: Thread.sleep(sleepInterval);
>
> This can be `2 * timeout`. It will be clearer to the reader that we just
> want to sleep longer than what the target is waiting.
Okay, modified as suggested.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129794282
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129794023
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129793675
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129793304
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129792900
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129792681
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129792361
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129791617
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129791132
PR Review Comment: https://git.openjdk.org/jdk/pull/30709#discussion_r3129790934
