On Tue, 22 Sep 2026 05:17:12 GMT, David Holmes <[email protected]> wrote:
>> The `GetThreadSnapshotHandshakeClosure` was added to support virtual threads >> in thread dumps. It is very similar to the logic used in the old >> safepoint-based thread dump. >> `GetThreadSnapshotHandshakeClosure::detect_locks` uses the raw >> `javaVFrame::monitors()` method rather than using the `locked_monitors()` >> method which already filters out some monitors including those for which >> `wait()` has been called. It uses the raw `monitors()` list because it wants >> to process eliminated compiled monitors itself, and they are already removed >> from `locked_monitors()`. But that means it should be doing its own >> filtering of monitors that are being waited-on so they are not reported as >> locked. This seems to have been an oversight with the original >> implementation. >> >> The logic is somewhat more complicated, particularly in the virtual thread >> case, because there are a number of points where we can dump the stack >> between `wait0` appearing as the top frame, and the point where we actually >> release the monitor. It would be wrong to hide the monitor from the "locked" >> section in that case. So we can still report that a thread is waiting-on a >> particular object and that it has locked that object, as that is the actual >> state of things. But the code no longer reports a released monitor as locked >> by the waiting thread. >> >> Note that `javaVFrame::locked_monitors` is also potentially imprecise in its >> reporting. It will report the monitor as locked until >> `current_waiting_monitor()` is set, and then elide it. However, as this code >> is only used when reporting platform threads this imprecision is not >> observable as a platform thread will not respond to a stack dump request in >> between the transition to in-VM (for the native call) and the release of the >> monitor. >> >> I also fixed a pre-existing typo whilst in this code. >> >> Testing >> - tiers 1-3 >> - updated tests in the PR >> >> Thanks >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Update the logic to check that the waiting thread has actually released the > monitor. > Update test accordingly to force release.x test/jdk/com/sun/management/HotSpotDiagnosticMXBean/DumpThreads.java line 347: > 345: assertNotNull(fields, "thread not found"); > 346: assertEquals("WAITING", fields.state()); > 347: assertFalse(contains(dumpForThread(tid, lines), "- > locked <" + lockAsString)); The plain text format thread is not specified. It's too fragile to be be trying to extract a "section". I think dumpForThread should be removed, a simple one line for search for the identity string of the lock object is sufficient here. The testing of the JSON format, just below, will do the right check of the owned monitors. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32977#discussion_r4068923653
