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

[Chris Plummer]

On Mon, 4 Mar 2024 11:51:01 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> 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.

>  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.

I tried my debug agent fixes without any fix in place for this problem and I'm not seeing any failures. I think it is because as I stated in the CR for this PR:

> However, a subsequent call to GetThreadState returns with the JVMTI_THREAD_STATE_INTERRUPTED bit set. Even so, the JVM does in fact properly consider the thread not to be interrupted. A 2nd call to RawMonitorWait does properly wait rather than immediately return with JVMTI_ERROR_INTERRUPT.

So when the debug agent gets interrupted during the RawMonitorWait and then re-enters RawMonitorWait, there is no issue. However, I also noted in the CR:

>  And one more thing to add is that at the java level calling Thread.isInterrupted() does return true for the virtual thread even though it should not. 

My test does check Thread.isInterrupted(), so there is the potential for it to get the wrong answer. However, this is done in the InterruptedException catch clause, and the java level interrupted flag is being cleared when the exception is raised, thus no problem here either.

So it turns out the only reason I found this issue is because I was seeing many different issues with the debug agent's interrupt handling, and noted this JVMTI issue while trying to debug the debug agent issues. But it turns out it probably was never causing problems for the debug agent.

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

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