On Wed, 12 Nov 2025 19:07:49 GMT, Daniel D. Daugherty <[email protected]> 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
