On Sat, 2 Mar 2024 07:58:40 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Please, review this fix correcting the JVMTI `RawMonitorWait()` >> implementation. >> The `RawMonitorWait()` is using the the `jt->is_interrupted(true)` to >> update the interrupt status of the interrupted waiting thread. The issue is >> that when it calls `jt->is_interrupted(true)` to fetch and clear the >> `interrupt status` of the virtual thread, this information is not propagated >> to the `java.lang.Thread` instance. >> In the `VirtualThread` class implementation the `interrupt status` for >> virtual thread is stored for both virtual and carrier threads. It is because >> the implementation of object monitors for virtual threads is based on >> pinning virtual threads, and so, always operates on carrier thread. The fix >> is to clear the interrupt status for both virtual and carrier >> `java.lang.Thread` instances. >> >> Testing: >> - tested with new test >> `hotspot/jtreg/serviceability/jvmti/vthread/InterruptRawMonitor` which is >> passed with the fix and failed without it >> - ran mach5 tiers 1-6 > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1510567016
