On Tue, 22 Jul 2025 05:47:37 GMT, David Holmes <[email protected]> wrote:
>> The `cv_internal_thread_to_JavaThread` method will now check if the thread
>> is a mounted virtual thread, and if so protect the carrier thread. User's of
>> the API that need to deal with virtual threads must still check the carrier
>> themselves as it could change asynchronously, but it is now guaranteed that
>> the carrier JavaThread returned via this method cannot terminate whilst
>> being used (the same as regular platform JavaThreads).
>>
>> It was noticed whilst updating the documentation that the `JvmtiExport`
>> variant `cv_oop_to_JavaThread` is unused and so can be removed.
>>
>> Testing:
>> - tiers 1-4
>>
>> Thanks
>
> David Holmes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move comment
src/hotspot/share/runtime/threadSMR.cpp line 839:
> 837: }
> 838: if (java_thread == nullptr) {
> 839: // Virtual thread was unmounted, or else carrier has now
> terminated.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26287#discussion_r2228017957
