8306324: StopThread results in thread being marked as interrupted, leading to unexpected InterruptedException

[sspitsyn]

If JVMTI StopThread is done when the thread is in certain various states (but not all states), after the async exception is delivered and handled, hotspot leaves the thread's interrupted flag set. The end result is the next time the thread does something like Thread.sleep(), it will immediately get an InterruptedException.

The fix is to clear the interrupted flag in the JavaThread::handle_async_exception() after an async pending exception has been set to be thrown with the set_pending_exception().

There are a couple of concerns with this fix which would be nice to sort out with reviewers:

1. The proposed fix may clear the interrupt state when it was already set prior to the issuing of the StopThread() (this concern was raised by @dholmes-ora in a comment of this JBS issue)
2. The impacted code path is shared between the class InstallAsyncExceptionHandshakeClosure used by the JVMTI StopThread implementation and the class ScopedAsyncExceptionHandshakeClosure used by the ScopedMemoryAccess

I feel that clearing the interrupted flag byt the JavaThread::handle_async_exception() is a right thing to do even though it was set before the call to JavaThread::install_async_exception(). Also, it has to be done for both StopThread and ScopedMemoryAccess.

The fix also includes minor tweaks of the test StopThreadTest to make the issue reproducible with it.

Testing:

• Mach5 tiers 1-6 are passed
• Ran the updated reproducer test hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8306324: StopThread results in thread being marked as interrupted, leading to unexpected InterruptedException (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26365/head:pull/26365
$ git checkout pull/26365

Update a local copy of the PR:
$ git checkout pull/26365
$ git pull https://git.openjdk.org/jdk.git pull/26365/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26365

View PR using the GUI difftool:
$ git pr show -t 26365

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26365.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back sspitsyn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@sspitsyn The following labels will be automatically applied to this pull request:

• hotspot-runtime
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (cd235460)

[dholmes-ora]

I need to dive into the code tomorrow but I can't help think that doing an actual interrupt is not really what is needed, we just need to unpark the thread if it is blocked ...

[AlanBateman]

I need to dive into the code tomorrow but I can't help think that doing an actual interrupt is not really what is needed, we just need to unpark the thread if it is blocked ...

I think we also need to step back and write down examples of where JVMTI StopThread is useful. The main use-case may be in the debugger where a thread is suspended at a breakpoint and the user wants to throw an exception to exercise some code path and exception handling. So I think it may be less about wakeup. Also if the target thread is in Object.wait then it can't continue until it re-enters the monitor so it's never been guaranteed to wakeup immediately. Although it may be unpalatable, I think we should look at having StopThread fail in these cases.

[pchilano]

I also don’t see the purpose of setting the interrupted flag if all we want is for the target to wake up if it’s in one of the blocking calls. From the 3 cases in JavaThread::interrupt(), the only one where I see setting that flag is needed is for Thread.sleep because we check it to bail out, otherwise we would just keep sleeping for the remaining time. But maybe we could also add a has_async_exception_condition() check to bailout.

[dholmes-ora]

But maybe we could also add a has_async_exception_condition() check to bailout.

Yes.

BTW I don't think this is in any way new. I think Thread.stop would have had the same issue. Issuing the internal interrupt was just a convenient way to unblock all the interruptible blocking points - and as Alan notes, monitor acquisition is not interruptible. We likely did not care that this could make the interrupt state appear odd if the thread continued after the async (ThreadDeath) exception. Not sure it is really that different for the debugger - if we have stopped the thread (not just suspended it) then we shouldn't really be expecting it to continue as normal afterwards.

[zhaosongzs]

/touch

[openjdk]

@zhaosongzs The pull request is being re-evaluated and the inactivity timeout has been reset.

[sspitsyn]

Thank you for the comments and suggestions!
I'll give it a try to get rid of the interrupt flag setting.

[AlanBateman]

@sspitsyn It might be useful to reach out to the IDEs to see what they are doing. From a quick test with IntelliJ then it appears to invoke both StopThread and InterruptThread when "Exception to Throw" is used. In that case, it means that Thread.sleep will wakeup, even if StopThread doesn't interrupt.

[sspitsyn]

@sspitsyn It might be useful to reach out to the IDEs to see what they are doing. From a quick test with IntelliJ then it appears to invoke both StopThread and InterruptThread when "Exception to Throw" is used. In that case, it means that Thread.sleep will wakeup, even if StopThread doesn't interrupt.

Good suggestion, thanks.