RFR: 8373366: HandshakeState should disallow suspend ops for disabler threads [v2]

[Patricio Chilano Mateo]

On Fri, 12 Dec 2025 11:44:28 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> 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