On Wed, 15 Jul 2026 16:28:24 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> An asynchronous exception sent by JVMTI `StopThread` to a virtual thread can
>> be processed and thrown at a point where it is unsafe to do so, leaving the
>> virtual thread in an invalid state. The unsafe nature of `StopThread` is not
>> an issue exclusive to virtual threads, but due to extra code executed during
>> mount/unmount transitions, some tests that are safe for platform threads are
>> not for virtual threads. In the reported bug, where the async exception is
>> sent to a thread executing `Thread.yield` in a loop, the exception ends up
>> being thrown on return from `VirtualThread.startTransition`. Since the
>> transition bits remain set, the virtual thread hits the reported assert in
>> `MountUnmountDisabler::start_transition` at the next unmount attempt.
>>
>> A similar issue can happen if the exception is thrown right after executing
>> `VirtualThread.endFirstTransition` or right before
>> `VirtualThread.startFinalTransition` which can be reproduced by calling
>> `StopThread` on virtual threads with empty tasks.
>>
>> The patch fixes these cases and potentially others that can happen if an
>> async exception is thrown while executing a method in the `VirtualThread`
>> class, i.e. it improves the robustness of the implementation in the presence
>> of async exceptions. It is not an attempt to make `StopThread` bulletproof
>> as that would be a more ambitious task.
>>
>> The proposed changes add two extra checks before installing the asynchronous
>> exception handshake. The first verifies that the top method is not a
>> `VirtualThread` method. The second verifies that the exception will be
>> thrown at the current bytecode, i.e. that exception processing will not be
>> deferred to a later safepoint poll where the target might already be in one
>> of the unsafe methods.
>>
>> A less restrictive alternative that avoids that second check is to have the
>> target defer processing of the handshake as long as it’s unsafe to do so (as
>> based on the first check above). If the handshake is still pending when an
>> unmount transition begins, we process it and save the exception in the
>> virtual thread’s `JvmtiThreadState` to be thrown at the end of the next
>> mount. I have a patch implementing this approach, but the code is a bit more
>> involved and I wasn’t convinced it was worth it.
>>
>> I also refactored `StopThread` by introducing `StopThreadClosure` and
>> `StopThreadAsyncClosure` handshake classes. This aligns it with the other
>> JVMTI methods that use `JvmtiHandshake`, and also keeps the `StopThread`
>> specific logic local to the JV...
>
> Patricio Chilano Mateo has updated the pull request with a new target base
> due to a merge or a rebase. The incremental webrev excludes the unrelated
> changes brought in by the merge/rebase. The pull request contains nine
> additional commits since the last revision:
>
> - extra space
> - Merge branch 'master' into JDK-8386116
> - Count async exceptions installed/thrown
> - Improve comments
> - Updates in libStopThreadTest2.cpp
> - More comments from David
> - Dan's comments
> - David's comments
> - v1
This looks good in general. I've posted a couple of questions/suggestions
though.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2406:
> 2404: // Checks for VirtualThread$VThreadContinuation$1.run
> 2405: (m->name() == vmSymbols::run_method_name() &&
> m->jvmti_hide_events()));
> 2406: }
Q: I have a concern that we only check the top frame while the execution can be
in a such frame context. I guess, we can skip it for now though. But what about
some Continuation methods which also can be in a transition? Should we add
something like this? :
if (holder == vmClasses::Continuation_klass()) {
return m->jvmti_hide_events();
}
test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest2/StopThreadTest2.java
line 57:
> 55: asyncThrownCounter.incrementAndGet();
> 56: }
> 57: }
Q: It seems the test provide coverage for yielding virtual threads. The PR
description also mentions unsafe delivery around `endFirstTransition`,
`startFinalTransition`, and empty virtual thread tasks. Does it make sense to
add coverage for these cases as well? I'm not sure if it worth to do though.
Just wanted to know your opinion.
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/31759#pullrequestreview-4712211566
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3594134520
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3594177289
