Skip to content

8361912: ThreadsListHandle::cv_internal_thread_to_JavaThread does not deal with a virtual thread's carrier thread #26287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+30 −66
Open

8361912: ThreadsListHandle::cv_internal_thread_to_JavaThread does not deal with a virtual thread's carrier thread #26287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8099035
Adapt thread conversion routines to return (and protect) the carrier
dholmes-ora Jul 14, 2025
4ed9035
Adapt ThreadSnapshotFactory::get_thread_snapshot to use the new API
dholmes-ora Jul 14, 2025
9e13596
Remove unused JvmtiExport::cv_oop_to_JavaThread API
dholmes-ora Jul 14, 2025
5d7eb80
Revert "Adapt ThreadSnapshotFactory::get_thread_snapshot to use the n…
dholmes-ora Jul 21, 2025
a195fe2
Revert to simpler fix without new API
dholmes-ora Jul 21, 2025
820f26e
Move comment
dholmes-ora Jul 22, 2025
d863add
Fix comments
dholmes-ora Jul 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 0 additions & 41 deletions src/hotspot/share/prims/jvmtiExport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,47 +859,6 @@ JvmtiExport::cv_external_thread_to_JavaThread(ThreadsList * t_list,
return JVMTI_ERROR_NONE;
}

// Convert an oop to a JavaThread found on the specified ThreadsList.
// The ThreadsListHandle in the caller "protects" the returned
// JavaThread *.
//
// On success, *jt_pp is set to the converted JavaThread * and
// JVMTI_ERROR_NONE is returned. On error, returns various
// JVMTI_ERROR_* values.
//
jvmtiError
JvmtiExport::cv_oop_to_JavaThread(ThreadsList * t_list, oop thread_oop,
JavaThread ** jt_pp) {
assert(t_list != nullptr, "must have a ThreadsList");
assert(thread_oop != nullptr, "must have an oop");
assert(jt_pp != nullptr, "must have a return JavaThread pointer");

if (!thread_oop->is_a(vmClasses::Thread_klass())) {
// The oop is not a java.lang.Thread.
return JVMTI_ERROR_INVALID_THREAD;
}
// Looks like a java.lang.Thread oop at this point.

JavaThread * java_thread = java_lang_Thread::thread(thread_oop);
if (java_thread == nullptr) {
// The java.lang.Thread does not contain a JavaThread * so it has
// not yet run or it has died.
return JVMTI_ERROR_THREAD_NOT_ALIVE;
}
// Looks like a live JavaThread at this point.

if (!t_list->includes(java_thread)) {
// Not on the JavaThreads list so it is not alive.
return JVMTI_ERROR_THREAD_NOT_ALIVE;
}

// Return a live JavaThread that is "protected" by the
// ThreadsListHandle in the caller.
*jt_pp = java_thread;

return JVMTI_ERROR_NONE;
}

class JvmtiClassFileLoadHookPoster : public StackObj {
private:
Symbol* _h_name;
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/prims/jvmtiExport.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,8 +454,6 @@ class JvmtiExport : public AllStatic {
jthread thread,
JavaThread ** jt_pp,
oop * thread_oop_p);
static jvmtiError cv_oop_to_JavaThread(ThreadsList * t_list, oop thread_oop,
JavaThread ** jt_pp);
};

// Support class used by JvmtiDynamicCodeEventCollector and others. It
Expand Down
30 changes: 24 additions & 6 deletions src/hotspot/share/runtime/threadSMR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,10 +791,16 @@ ThreadsListHandle::~ThreadsListHandle() {
// associated ThreadsList. This ThreadsListHandle "protects" the
// returned JavaThread *.
//
// If the jthread resolves to a virtual thread then the JavaThread *
// for its current carrier thread (if any) is returned via *jt_pp.
// It is up to the caller to prevent the virtual thread from changing
// its mounted status, or else account for it when acting on the carrier
// JavaThread.
//
// If thread_oop_p is not null, then the caller wants to use the oop
// after this call so the oop is returned. On success, *jt_pp is set
// after this call so the oop is always returned. On success, *jt_pp is set
// to the converted JavaThread * and true is returned. On error,
// returns false.
// returns false, and *jt_pp is unchanged.
//
bool ThreadsListHandle::cv_internal_thread_to_JavaThread(jobject jthread,
JavaThread ** jt_pp,
Expand All @@ -818,10 +824,22 @@ bool ThreadsListHandle::cv_internal_thread_to_JavaThread(jobject jthread,

JavaThread *java_thread = java_lang_Thread::thread_acquire(thread_oop);
if (java_thread == nullptr) {
// The java.lang.Thread does not contain a JavaThread* so it has not
// run enough to be put on a ThreadsList or it has exited enough to
// make it past ensure_join() where the JavaThread* is cleared.
return false;
if (!java_lang_VirtualThread::is_instance(thread_oop)) {
// The java.lang.Thread does not contain a JavaThread* so it has not
// run enough to be put on a ThreadsList or it has exited enough to
// make it past ensure_join() where the JavaThread* is cleared.
return false;
} else {
// For virtual threads we need to extract the carrier's JavaThread - if any.
oop carrier_thread = java_lang_VirtualThread::carrier_thread(thread_oop);
if (carrier_thread != nullptr) {
java_thread = java_lang_Thread::thread(carrier_thread);
}
if (java_thread == nullptr) {
// Virtual thread was unmounted, or else carrier has now terminated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If java_thread for a virtual thread is null then this thread is unmounted. The words about the terminated carrier thread are confusing.

I have one concern here. Nothing protects this code from facing some unexpected conditions as this function is racy with mounting and unmounting of target virtual thread

  • this function can observe a JavaThread in mount or unmount transition where the thread identity of a java.lang.Thread associated with the JavaThread is not consistent (in a VTMS transition the thread identity might not match the stack trace)
  • nothing guaranties that the result returned by this function remains the same upon return as mount/unmount of the target virtual thread can be executed concurrently: a mounted virtual thread can be quickly unmounted or an unmounted virtual thread can be quickly mounted

So, it seems that this function is going to work correctly if used for target platform threads only or if the JavaThread* pointer is not really needed. Otherwise, the association with the JavaThread needs to be rechecked and its stability somehow guaranteed with any form of scheduling suspension or a VTMS transition disabler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The java_thread can be null because the carrier of a mounted vthread was not contained in the ThreadsListHandle.

The only thing this function guarantees is that if the vthread appeared to have a carrier and that carrier is protected by the TLH, then the caller can safely interact with the carrier knowing it can't terminate - same as for regular threads. The caller has to account for the potential async mounting/unmounting of vthread - e.g. by handshaking the reported carrier and then confirming it is still the carrier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that java_thread may already be null so we don't get to execute line 836.

Copy link
Contributor

@sspitsyn sspitsyn Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, David. The potential issue I'm still concerned about is that a subsequent handshaking can also observe the JavaThread (and virtual thread as well) in a VTMS transition when there is no protection with a VTMSTransitionDisabler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that java_thread may already be null so we don't get to execute line 836.

Right. I guess, this answers my comment about the line 839.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once in the handshake the carrier (and thus virtual thread) is "frozen" with regards to any transition. It could be anywhere in the process of mounting/unmounting (depending of course exactly where the carrier might respond to the handshake request), and the actual handshake operation has to deal with that. But that is true no matter how you extracted a reference to the carrier. As I said before the only thing this change guarantees is that the carrier is protected by the TLH and can't fully terminate whilst handshaking with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this change is in main line then we can sync'up the loom repo and work on the follow-up changes. The loom repo has changes (that are not in main line) for "suspending" an unmounted thread. For the mounted case then we'll need checks in the handshake to ensure that the expected virtual threads is mounted. We'll need stress tests of course and we can collaborate there in advance of proposing changes for main line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that java_thread may already be null so we don't get to execute line 836.

I'm having some trouble with this statement. Working backwards from L836, I see
the nullptr check on L826 and the check for non-virtual thread and bail on L827
and L831. However, if we saw nullptr on L826 and we have a virtual thread, then
we can get to L836 even when we started off with a null java_thread.

So I guess I'm not sure what point you're trying to make with the statement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then we can get to L836 even when we started off with a null java_thread.

Sure. My point was more the other way round - we could have a null JavaThread without the null coming from line 836.

The comment was just trying to expand on my previous comment.

return false;
}
}
}
// Looks like a live JavaThread at this point.

Expand Down
23 changes: 6 additions & 17 deletions src/hotspot/share/runtime/threadSMR.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2017, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -45,8 +45,8 @@ class ThreadsList;
// operation. It is no longer necessary to hold the Threads_lock to safely
// perform an operation on a target thread.
//
// There are several different ways to refer to java.lang.Thread objects
// so we have a few ways to get a protected JavaThread *:
// There are two ways to refer to java.lang.Thread objects so we have two ways
// to get a protected JavaThread*:
//
// JNI jobject example:
// jobject jthread = ...;
Expand All @@ -69,21 +69,10 @@ class ThreadsList;
// }
// : // do stuff with 'jt'...
//
// JVM/TI oop example (this one should be very rare):
// oop thread_obj = ...;
// :
// JavaThread *jt = nullptr;
// ThreadsListHandle tlh;
// jvmtiError err = JvmtiExport::cv_oop_to_JavaThread(tlh.list(), thread_obj, &jt);
// if (err != JVMTI_ERROR_NONE) {
// return err;
// }
// : // do stuff with 'jt'...
//
// A JavaThread * that is included in the ThreadsList that is held by
// a ThreadsListHandle is protected as long as the ThreadsListHandle
// remains in scope. The target JavaThread * may have logically exited,
// but that target JavaThread * will not be deleted until it is no
// remains in scope. The target JavaThread* may have logically exited,
// but that target JavaThread* will not be deleted until it is no
// longer protected by a ThreadsListHandle.
//
// SMR Support for the Threads class.
Expand Down Expand Up @@ -318,7 +307,7 @@ class ThreadsListHandle : public StackObj {
inline Iterator begin();
inline Iterator end();

bool cv_internal_thread_to_JavaThread(jobject jthread, JavaThread ** jt_pp, oop * thread_oop_p);
bool cv_internal_thread_to_JavaThread(jobject jthread, JavaThread** jt_pp, oop* thread_oop_p);

bool includes(JavaThread* p) {
return list()->includes(p);
Expand Down