[jdk17] RFR: 8271251: JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6" [v2]

[Daniel D.Daugherty]

On Mon, 26 Jul 2021 22:09:57 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> src/hotspot/share/runtime/handshake.cpp line 615:
>> 
>>> 613:   assert(_lock.owned_by_self(), "Lock must be held");
>>> 614:   assert(!_handshakee->has_last_Java_frame() || _handshakee->frame_anchor()->walkable(), "should have walkable stack");
>>> 615: 
>> 
>> Can we add an assert:
>> 
>> `assert(_handshakee->thread_state() == _thread_blocked, "Caller should have transitioned to _thread_blocked);
>> `
>> ?
>
> Done!

Should be able to add that assert. The new code has only two callers
to do_self_suspend() and both make sure we are in `_thread_blocked`.

>> src/hotspot/share/runtime/handshake.cpp line 616:
>> 
>>> 614:   assert(!_handshakee->has_last_Java_frame() || _handshakee->frame_anchor()->walkable(), "should have walkable stack");
>>> 615: 
>>> 616:   log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " suspended", p2i(_handshakee));
>> 
>> I think I prefer this logging to remain in the while loop. By placing it there you know the thread will actually wait() and you can also see if there are multiple suspend requests. It might even make sense to put the resume log statement in the loop after the wait() with an explicit check of is_suspended() to preclude a spurious wakeup.
>
> So for the resume log statement, once the target wakes up from wait() the _suspended flag could be set not necessarily because of a spurious wakeup but because another JT suspended the target right after the resume. But I can add a log statement in HandshakeState::resume() to track all resumes. 
> As for the suspend log statement I thought about not moving it but then I saw that we are already logging suspends in HandshakeState::suspend_with_handshake(), and since all these logging statements are done under _lock held there is already a sequence of suspend/resume events (if adding log in HandshakeState::resume()). 
> What do you think?

I would not change any of the logging in the JDK17 fix unless there's a very
good reason. We want the JDK17 fix to be as minimal as possible.

-------------

PR: https://git.openjdk.java.net/jdk17/pull/283