RFR: 8325187: JVMTI GetThreadState says virtual thread is JVMTI_THREAD_STATE_INTERRUPTED when it no longer is

[Serguei Spitsyn]

On Mon, 4 Mar 2024 04:09:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/runtime/javaThread.cpp line 594:
>> 
>>> 592:       // thread_oop is virtual, clear carrier thread interrupt status as well
>>> 593:       java_lang_Thread::set_interrupted(threadObj(), false);
>>> 594:     }
>> 
>> This isn't right for the case that Thread::interrupt it called around the same time.  The interrupt status on both the virtual thread and the carrier need to be kept in sync so the changes here need to work like an up call to VirtualThread.getAndClearInterrupt and use the interrupt lock.
>
> On that note, note that the entire comment block preceding this needs to be updated for virtual threads. And it is far from clear to me how to correctly manage the interrupt state of virtual threads within the VM, when it requires locking and coordination across two threads! Performing a Java upcall to manage this could be very problematic from within ObjectMonitor::wait. I think at the moment pinning resolves this because the carrier thread interrupt status is a proxy for the virtual thread's status, when it is waiting. Once we remove pinning however this gets even more complex. Though without pinning things should also simplify greatly because we should not need to distinguish between the virtual thread and the carrier thread ... but perhaps I'm getting ahead of myself here. But I would hate for us to come up with a solution here and now that has to jump through hoops to make things work, if in the not-do-distant future, removal of pinning make this much simpler.

Alan and David, thank you for the comments!
The initial implementation of the `is_iterrupt()` intentionally avoided any synchronization and allowed a potential raises with the concurrent interrupts. Please, see the comment above at lines: `573-587`. It is why David mentioned the need to update the comment. I agree with Alan that that interrupt status of virtual and carrier threads have to be kept in sync. But as we already discussed with Alan before, an upcall to Java does not looks good and can cause some issues on the JDWP side. I'm thinking to model a CAS kind of operation to keep the two interrupt statuses in sync but still need to prove it is going to be safe. Also, there is a question if it worth the complexity.
The need to keep interrupt status in both virtual and carrier threads comes from virtual thread pinning. I believe, there would be no need to keep interrupt status for both threads in the Java object monitors inplementations. AFAICS, then the implementation of the `is_interrupted()` can be kept as it is now. 
So, it looks like a good suggestion to wait for Java object monitors. However, the JDWP related fix depends on this and will need to wait as well.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1510882826