Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+10 −10
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cd23546
8306324: StopThread results in thread being marked as interrupted, le…
sspitsyn Jul 17, 2025
8338fb5
Merge
sspitsyn Jul 28, 2025
fde881f
review: implemented a suggestion: do not set interrupt flag at all
sspitsyn Jul 28, 2025
1f7b67c
review: remove tweak from JavaThread::sleep_nanos
sspitsyn Jul 29, 2025
a46ef6f
review: restored the tweak in the JavaThread::sleep_nanos
sspitsyn Jul 30, 2025
476665c
review: correct comment in sleep_nanos and avoid trowing IE in JVM_Sl…
sspitsyn Jul 31, 2025
11bc5a7
fixed typo: in latest update
sspitsyn Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/hotspot/share/prims/jvm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2893,15 +2893,16 @@ JVM_ENTRY(void, JVM_SleepNanos(JNIEnv* env, jclass threadClass, jlong nanos))
} else {
ThreadState old_state = thread->osthread()->get_state();
thread->osthread()->set_state(SLEEPING);
if (!thread->sleep_nanos(nanos)) { // interrupted
if (!thread->sleep_nanos(nanos)) { // interrupted or async exception was installed
// An asynchronous exception could have been thrown on
// us while we were sleeping. We do not overwrite those.
if (!HAS_PENDING_EXCEPTION) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not for this bug but we have this HAS_PENDING_EXCEPTION check here and further up but I don't see how we can have a pending exception when calling this method. Based on the comment here seems we just wanted to check the async ones as added now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always have HAS_PENDING_EXCEPTION == true if async exception was installed?
If so, then this newly added check is not really needed:

           if (!thread->has_async_exception_condition()) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we process the async exception handshake operation, HAS_PENDING_EXCEPTION will be false. The only way for HAS_PENDING_EXCEPTION to be true would be if we already entered the method with a pending exception, but I don’t see how that is possible.

HOTSPOT_THREAD_SLEEP_END(1);

// TODO-FIXME: THROW_MSG returns which means we will not call set_state()
// to properly restore the thread state. That's likely wrong.
THROW_MSG(vmSymbols::java_lang_InterruptedException(), "sleep interrupted");
if (!thread->has_async_exception_condition()) {
// TODO-FIXME: THROW_MSG returns which means we will not call set_state()
// to properly restore the thread state. That's likely wrong.
THROW_MSG(vmSymbols::java_lang_InterruptedException(), "sleep interrupted");
}
}
}
thread->osthread()->set_state(old_state);
Expand Down
6 changes: 4 additions & 2 deletions src/hotspot/share/runtime/javaThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,6 @@ void JavaThread::install_async_exception(AsyncExceptionHandshakeClosure* aehc) {
oop vt_oop = vthread();
if (vt_oop == nullptr || !vt_oop->is_a(vmClasses::BaseVirtualThread_klass())) {
// Interrupt thread so it will wake up from a potential wait()/sleep()/park()
java_lang_Thread::set_interrupted(threadObj(), true);
this->interrupt();
}
}
Expand Down Expand Up @@ -2098,7 +2097,7 @@ bool JavaThread::sleep(jlong millis) {

// java.lang.Thread.sleep support
// Returns true if sleep time elapsed as expected, and false
// if the thread was interrupted.
// if the thread was interrupted or async exception was installed.
bool JavaThread::sleep_nanos(jlong nanos) {
assert(this == Thread::current(), "thread consistency check");
assert(nanos >= 0, "nanos are in range");
Expand All @@ -2118,6 +2117,9 @@ bool JavaThread::sleep_nanos(jlong nanos) {
jlong nanos_remaining = nanos;

for (;;) {
if (has_async_exception_condition()) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above JavaThread::sleep_nanos should be updated about this case.
Also, back in JVM_SleepNanos we should probably skip throwing IE for this case (even though it will be overwritten by the async one later).

Copy link
Contributor Author

@sspitsyn sspitsyn Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, addressed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mach5 tiers 1-6 are good with the latest pushes.

}
// interruption has precedence over timing out
if (this->is_interrupted(true)) {
return false;
Expand Down
3 changes: 0 additions & 3 deletions test/hotspot/jtreg/serviceability/jvmti/vthread/StopThreadTest/StopThreadTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ public void run() {
log("TestTask.run: caught expected AssertionError from method A()");
seenExceptionFromA = true;
}
Thread.interrupted();
if (!seenExceptionFromA && !preemptableVirtualThread()) {
StopThreadTest.setFailed("TestTask.run: expected AssertionError from method A()");
}
Expand All @@ -224,7 +223,6 @@ public void run() {
log("TestTask.run: caught expected AssertionError from method B()");
seenExceptionFromB = true;
}
Thread.interrupted();
if (!seenExceptionFromB) {
StopThreadTest.setFailed("TestTask.run: expected AssertionError from method B()");
}
Expand All @@ -237,7 +235,6 @@ public void run() {
log("TestTask.run: caught expected AssertionError from method C()");
seenExceptionFromC = true;
}
Thread.interrupted();
if (!seenExceptionFromC) {
StopThreadTest.setFailed("TestTask.run: expected AssertionError from method C()");
}
Expand Down