RFR: 8316397: StackTrace/Suspended/GetStackTraceSuspendedStressTest.java failed with: SingleStep event is NOT expected [v4]

[Patricio Chilano Mateo]

On Thu, 3 Apr 2025 10:10:44 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>>> Let me see if I have the sequence of events correct:
>>>  - we have a thread leaving Java to enter the VM, in this case to post a single-step event, which is not suspended
>>>   - when trying to acquire the JvmtiThreadState_lock the thread blocks and becomes handshake/safepoint-safe and is then suspended
>>>   - the thread acquires the lock and goes to post the event but because it has now been suspended it is an error to do so
>> 
>> This is almost right but let me add a couple of details.
>> A suspend request was processed/registered/accepted at the handshake/safepoint-safe point but physical suspension in the `ThreadSelfSuspensionHandshake` has not happened yet which created a problem. Also, the problem is not that the thread goes to post the event. Suspension and event posting are racy by design. The problem is that the `SingleStep` events for this thread are also enabled at the handshake/safepoint-safe point after the suspend request was processed/registered/accepted.
>> 
>>> So what we need is a way to disable suspension so that the transient/temporary switch to _thread_blocked does not allow a suspend request to be processed. I think that is preferable to trying to find where to add a place to check for suspension and actually suspend it.
>> 
>> I'm also thinking toward this direction. The best way to fix this would be to disallow processing/accepting new suspend requests in `MutexLocker mu(JvmtiThreadState_lock)` which is executed in context of `get_jvmti_thread_state()` call:
>> 
>> inline JvmtiThreadState* JvmtiThreadState::state_for(JavaThread *thread, Handle thread_handle) {
>>   // In a case of unmounted virtual thread the thread can be null.
>>   JvmtiThreadState* state = thread_handle == nullptr ? thread->jvmti_thread_state() :
>>                                                 java_lang_Thread::jvmti_thread_state(thread_handle());
>>   if (state == nullptr) {
>>     MutexLocker mu(JvmtiThreadState_lock);
>>   . . .
>> 
>> Unfortunately, it looks like it is tricky and risky to do.
>
> David, we had an internal Zoom meeting with Patricio and agreed on a couple of points (I hope, Patricio will fix me if needed).
> #1:
> The main (biggest) problem is the call to `JvmtiEventController::thread_started()` in the function `get_jvmti_thread_state()`. The function `JvmtiThreadState::state_for()` is called there if a mounted virtual thread is the current thread and any thread-filtered event is enabled globally. The function `JvmtiThreadState::state_for()` has this line: `MutexLocker mu(JvmtiThreadState_lock);`
> 
> The `JvmtiThreadState_lock` allows safepointing. It is where a JVMTI suspend request for this thread has been processed/registered. The `SingleStep` events for this thread are also enabled after the suspend request was processed/registered. However, suspends are not allowed by this `MutexLocker`.
> This is very common code path for any thread-filtered event (e.g. the event can be enabled for a specific thread).
> 
> #2:
> This PR fixes this exact issue with this update:
> 
> JvmtiThreadState* JvmtiExport::get_jvmti_thread_state(JavaThread *thread) {
>   assert(thread == JavaThread::current(), "must be current thread");
>   if (thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr) {
>     JvmtiEventController::thread_started(thread);
> +    if (thread->is_suspended()) {
> +      // suspend here if there is a suspend request
> +      ThreadBlockInVM tbivm(thread);
> +    }
>   }
>   . . .
> 
> 
> But we also agreed to add a tweak for the function `post_dynamic_code_generated_while_holding_locks()` to disallow suspension in this context. Then it will look like this:
> 
> +JvmtiThreadState* JvmtiExport::get_jvmti_thread_state(JavaThread *thread, bool allow_suspend) {
>   assert(thread == JavaThread::current(), "must be current thread");
>   if (thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr) {
>     JvmtiEventController::thread_started(thread);
> +    if (allow_suspend && thread->is_suspended()) {
> +      // suspend here if there is a suspend request
> +      ThreadBlockInVM tbivm(thread, true /* allow suspend */);
> +    }
>   }
>   . . .
> 
> void JvmtiExport::post_dynamic_code_generated_while_holding_locks(const char* name,
>                                                                   address code_begin, address code_end)
> {
>   . . .
>   JvmtiThreadState *state = get_jvmti_thread_state(thread, false /* allow_suspend */);
>   . . .
> 
> 
> I have already pushed the minor tweak above.
> 
> #3:
> We also agreed to investigate more case where a similar situation can occur within...

Thanks for the summary Serguei. I would only add that IMO, given that it’s already clear there could be many suspend points before even reaching one of this `JvmtiExport::post_*` methods (like the one shown in #3), I would just address all these cases here too my moving the suspend check outside of the `thread->is_vthread_mounted() && thread->jvmti_thread_state() == nullptr` conditional. But since this PR has already been blocked for too long I’m okay if we only want to address the specific suspend point in #1.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23490#discussion_r2027494695