On Thu, 31 Jul 2025 03:45:10 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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`
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fixed typo: in latest update
Thanks Serguei, looks good to me.
src/hotspot/share/prims/jvm.cpp line 2899:
> 2897: // An asynchronous exception could have been thrown on
> 2898: // us while we were sleeping. We do not overwrite those.
> 2899: if (!HAS_PENDING_EXCEPTION) {
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.
-------------
Marked as reviewed by pchilanomate (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26365#pullrequestreview-3075988282
PR Review Comment: https://git.openjdk.org/jdk/pull/26365#discussion_r2245769502
