On Fri, 12 Dec 2025 11:44:28 GMT, Serguei Spitsyn <[email protected]> wrote:
>> src/hotspot/share/runtime/handshake.cpp line 524:
>>
>>> 522: assert(allow_suspend || !check_async_exception, "invalid case");
>>> 523: #if INCLUDE_JVMTI
>>> 524: if (allow_suspend && (_handshakee->is_disable_suspend() ||
>>> _handshakee->is_vthread_transition_disabler())) {
>>
>> I see we need this because a thread could be suspended outside a disable
>> operation (e.g. waiting in `disable_transition_for_one()`) but only process
>> the suspend once inside it. Looking at the places where we use
>> `MountUnmountDisabler`, the one where I can clearly see this could happen is
>> `JvmtiEnv::InterruptThread` because of the upcall to Java. Did you encounter
>> any other places where we could process suspend requests inside a disabling
>> operation?
>
> Not sure, I correctly understand your question. Are you asking about possible
> suspend points inside a `MountUnmountDisabler` scope? It is used for
> implementation of the JVMTI functions which have a deal with threads. They
> normally have suspend points (points where an asynchronous suspend operation
> can be picked and executed by the target thread itself with
> `HandshakeState::process_by_self()`), for instance, because they use
> handshakes.
> As I understand, the `ThreadSelfSuspensionHandshakeClosure` is an
> `AsyncHandshakeClosure`. As the suspending operation is asynchronous, the
> suspending thread can finish its suspending work and get out of the exclusive
> operation scope. Then the target thread can enter its own
> `MountUnmountDisabler` scope and get suspended there. The tweak above is to
> disallow such a scenario.
Right, I was just curious whether you ran into this issue or only found it by
code inspection. But I'm guessing you found disablers processing
`ThreadSelfSuspensionHandshakeClosure` handshakes because of the other issue in
`enable_transition_for_all()`, and that accidentally led you to consider this
other potential case where a thread is suspended before entering a
`MountUnmountDisabler` scope but processes the async handshake inside it?
Thanks for updating the PR description.
>> src/hotspot/share/runtime/javaThread.cpp line 1182:
>>
>>> 1180: bool JavaThread::java_suspend(bool register_vthread_SR) {
>>> 1181: // Suspending a vthread transition disabler can cause deadlocks.
>>> 1182: assert(!is_vthread_transition_disabler(), "no suspend allowed for
>>> vthread transition disablers");
>>
>> So except for self-suspend, how can we hit this assert if we use an
>> exclusive disabler? And if we remove it and allow a disabler to
>> self-suspend, wouldn't we deadlock since the resumer would be unable to run?
>
> You are asking right question, thanks!
> I've found and fixed the issue caused this and also restored the assert.
> I will also need to update the PR description to reflect this change.
Thanks! Now it makes sense why we were hitting this assert. Since that was
introduced in 8364343 I think we should separate that fix and backport it to 26.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28740#discussion_r2614963770
PR Review Comment: https://git.openjdk.org/jdk/pull/28740#discussion_r2614961974
