On Mon, 4 Mar 2024 09:45:59 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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. David can correct me, but I think the only cases in the VM where a thread may clear its interrupt status is Thread.interrupted, Thread.sleep, Object.wait, and JVMTI RawMonitorWait. Thread.interrupted and Thread.sleep are different implementation and not interesting here. Right now, Object.wait does wait on the carrier. If the virtual thread is interrupted while in the monitor's wait set then InterruptedException will be thrown when the thread reenters and the interrupt status of both threads will be cleared at that point (it will have of course have been cleared by the VM too prior to throwing). If something sneaky were to go behind our backs and interrupt the carrier directly then it will be benign for this case but might cause a spurious InterruptedException at other times, say where the mounted virtual thread were to call Object.wait soon after its carrier was interrupted directly. We've mostly ignored that concern. For JVMTI RawMonitorWait then it has to coordinate with Thread.interrupt and JVMTI InterruptThread. The latter does do an upcall when the target is a virtual thread so it's the same as Thread.interrupt. So minimally RawMonitorWait will need to disable suspend and and clear the interrupt status of both threads while holding the interrupt lock. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18093#discussion_r1511036065
