On Fri, 3 Jul 2026 00:49:43 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 JVMTI code.
>
> The changes...
I have mostly nits, but there's one substantive comment
about a null-check in the wrong place.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2413:
> 2411:
> 2412: class StopThreadAsyncClosure : public AsyncExceptionHandshakeClosure {
> 2413: public:
Nit: Needs a one space indent.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2458:
> 2456: _result = JVMTI_ERROR_OPAQUE_FRAME;
> 2457: return;
> 2458: }
The `!JvmtiEnvBase::is_vthread_suspended(target_h(), _target_jt)` on L2450
above passes `_target_jt` and that function can dereference `_target_jt`.
Perhaps the `nullptr` check should be first.
src/hotspot/share/prims/jvmtiEnvBase.hpp line 537:
> 535:
> 536: // HandshakeClosure to send an asynchronous exception to thread.
> 537: class StopThreadClosure : public JvmtiUnitedHandshakeClosure {
Not to `thread`. This should be to `target`.
src/hotspot/share/prims/jvmtiEnvBase.hpp line 538:
> 536: // HandshakeClosure to send an asynchronous exception to thread.
> 537: class StopThreadClosure : public JvmtiUnitedHandshakeClosure {
> 538: private:
Nit: Needs a one space indent.
src/hotspot/share/prims/jvmtiEnvBase.hpp line 540:
> 538: private:
> 539: Handle _exception;
> 540: public:
Nit: Needs a one space indent.
src/hotspot/share/prims/jvmtiEnvBase.hpp line 543:
> 541: StopThreadClosure(JavaThread* thread, oop exception)
> 542: : JvmtiUnitedHandshakeClosure("StopThread"),
> 543: _exception(thread, exception) {}
Nit: I think `thread` should `current_thread` in two places.
src/hotspot/share/runtime/interfaceSupport.inline.hpp line 129:
> 127:
> 128: class ThreadInVMfromJava : public ThreadStateTransition {
> 129: AtNoAsyncEntryMark nam;
Nit: usually these variable names match the uppercase
case part of the helper class, e.g.: `anaem` instead of `nam`.
src/hotspot/share/runtime/javaThread.hpp line 1359:
> 1357: public:
> 1358: AtNoAsyncEntryMark(JavaThread *t, bool b)
> 1359: : _target(t), _count(!b) {
Nit: I usually use `jt` for a JavaThread* var and `t` for a Thread* var.
I'm having trouble with the `b` parameter name that is assigned to `_count`
after negation.
Why is this naming so obscure? Perhaps `_do_count` for the local and something
better
for `b`?
test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest2/libStopThreadTest2.cpp
line 48:
> 46: Java_StopThreadTest2_stopThread(JNIEnv *jni, jclass cls, jthread thread,
> jobject exception) {
> 47: jvmtiError err = jvmti->StopThread(thread, exception);
> 48: if (err != JVMTI_ERROR_OPAQUE_FRAME) {
A 1-line comment about why it is okay to ignore OPAQUE_FRAME would be good.
-------------
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/31759#pullrequestreview-4647207545
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538309584
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538365549
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538392840
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538306969
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538308030
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538325982
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538214609
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538233768
PR Review Comment: https://git.openjdk.org/jdk/pull/31759#discussion_r3538807410
