On Thu, 24 Jul 2025 12:29:29 GMT, David Holmes <[email protected]> wrote:
>> 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.
>
> 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.
Note that `java_thread` may already be null so we don't get to execute line 836.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26287#discussion_r2228385947
