JVMTI WaitNotifySuspendedVThreadTest fails with JVMTI_ERROR_THREAD_SUSPENDED

[dipak-bagadiya]

Issue

The check_jvmti_status function returns an error, JVMTI_ERROR_THREAD_SUSPENDED, while the main thread attempts to suspend the virtual and carrier threads.

Test CMD

make test TEST="jtreg:test/hotspot/jtreg/serviceability/jvmti/vthread/WaitNotifySuspendedVThreadTest" JTREG="JAVA_OPTIONS=--enable-preview -Dvm.continuations=true;VERBOSE=all"

Test Output

STDOUT:
setting event callbacks ...
setBreakpoint: started
setBreakpoint: finished
Breakpoint: before monitor.wait(): methBreakpoint in virtual thread
Main thread: suspending virtual and carrier threads
check_jvmti_status: JVMTI function returned error: JVMTI_ERROR_THREAD_SUSPENDED (14)
STDERR:

Fatal error: SuspendThread thread

[babsingh]

Problem

JVMTI treats a virtual thread and its carrier thread as two separate threads. If JVMTI suspends a virtual thread, then its carrier thread should not be suspended and vice-versa. Similarly, if JVMTI resumes a virtual thread, then its carrier thread should not be resumed and vice-versa.

This is only a front-end change to satisfy the JVMTI specification. Experiments with the RI have shown that both the mounted virtual thread and carrier thread stop working under the hood when either thread is suspended.

In OpenJ9, currently, a mounted virtual thread and its carrier thread share the same J9VMThread. The thread state is derived from the J9VMThread. Due to the sharing of the J9VMThread in the mounted state, if a mounted virtual thread is suspended, then the JVMTI functions will also reflect that its carrier thread is suspended. Other scenarios where the sharing of the J9VMThread will impact the behaviour of JVMTI functions:

• if a carrier thread is suspended, then the mounted virtual thread will show up as suspended;
• if a mounted virtual thread is resumed, then its carrier thread will show up as resumed; and
• if a carrier thread is resumed, then the mounted virtual reflect as resumed.

Solution

1. Inject isSuspendedByJVMTI in all java.lang.Thread instances. Currently, it is only injected for java.lang.VirtualThread instances:

        === jclinit.c ====
	/* Stores a non-zero value if the virtual thread is suspended by JVMTI. */
	if (0 != vmFuncs->addHiddenInstanceField(vm, "java/lang/Thread", "isSuspendedByJVMTI", "I", &vm->isSuspendedByJVMTIOffset)) {
		return 1;
	}

2. Don't reset it to 0 in javanextvmi.cpp; it will only be reset once the thread is resumed:

	/* If isSuspendedByJVMTI is non-zero, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND is set
	 * in currentThread->publicFlags while resetting isSuspendedByJVMTI to zero. During
	 * mount, this suspends the thread if the thread was unmounted when JVMTI suspended it.
	 */
	if (0 != J9OBJECT_U32_LOAD(currentThread, threadObj, vm->isSuspendedByJVMTIOffset)) {
		J9OBJECT_U32_STORE(currentThread, threadObj, vm->isSuspendedByJVMTIOffset, 0); // REMOVE
		vmFuncs->setHaltFlag(currentThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND);
	}

3. Update jvmtiGetThreadState:
- SUSPENDED state should be derived from isSuspendedByJVMTI, and not from the shared J9VMThread.

4. Update jvmtiThread.c:resumeThread:
- It should reset isSuspendedByJVMTI to 0 for the thread instance.
- J9VMThread halt flag should only be cleared if the isSuspendedByJVMTI field is 0 for both the mounted virtual thread and corresponding carrier thread.

5. Update suspendhelper.cpp:suspendThread:
- It should set isSuspendedByJVMTI to a non-zero value for the thread instance.
- If the halt flag is already set in the J9VMThread, then it should just set the isSuspendedByJVMTI field to a non-zero value.

[babsingh]

@dipak-bagadiya #16689 (comment) contains the draft solution.

@gacholio Can you please give feedback on the above solution?

[gacholio]

For normal threads, there is only one suspension state, i.e. you can suspend a thread in java and resume it using JVMTI. Also, jvmtiGetThreadState must continue working for threads suspended in java.

Perhaps a better solution would be to maintain a suspended flag in the object that is not specific to JVMTI and set/clear it in the java suspend/resume as well.

There are also uses of J9_PUBLIC_FLAGS_HALT_THREAD_ANY_NO_JAVA_SUSPEND that may not work properly.

[babsingh]

Perhaps a better solution would be to maintain a suspended flag in the object that is not specific to JVMTI and set/clear it in the java suspend/resume as well. There are also uses of J9_PUBLIC_FLAGS_HALT_THREAD_ANY_NO_JAVA_SUSPEND that may not work properly.

Agreed for carrier threads. This does not apply to virtual threads since they cannot be resumed or suspended from Java code.

We can rename isSuspendedByJVMTI to isSuspendedInternal, and it can also be updated from Java suspend/resume:

• openj9/runtime/jcl/common/thread.cpp

  Line 188 in c50acf4

  Java_java_lang_Thread_resumeImpl(JNIEnv *env, jobject rcv)

• openj9/runtime/jcl/common/thread.cpp

  Line 207 in c50acf4

  Java_java_lang_Thread_suspendImpl(JNIEnv *env, jobject rcv)

The above fix will only be needed for JDK19's java/lang/Thread.

In JDK20's java/lang/Thread, Thread.resume, Thread.suspend and Thread.stop are all unsupported operations. The above concern won't apply to JDK20 and onwards. Also, the proposed fix will only be ready for JDK20.

[babsingh]

Revised Suspend, Resume and GetThreadState Logic

The old logic in #16689 (comment) is flawed. The new logic is below. @dipak-bagadiya Let me know if there any questions.

SuspendThread(thread) and Java_java_lang_Thread_suspendImpl:
	j9object_t threadObject = J9_JNI_UNWRAP_REFERENCE(thread);
	targetThread is derived using getVMThread(...)
	if ((NULL != targetThread) && (threadObject == targetThread->threadObject)) {
		/* The thread is running. */
		Set the halt flag
	}

	Set thread.isSuspendedInternal to 1

ResumeThread(thread) and Java_java_lang_Thread_resumeImpl:
	j9object_t threadObject = J9_JNI_UNWRAP_REFERENCE(thread);
	targetThread is derived using getVMThread(...)
	if ((NULL != targetThread) && (threadObject == targetThread->threadObject)) {
		/* The thread was running during suspend. */
		Clear the halt flag
	}
	 
	Set thread.isSuspendedInternal to 0


VirtualThreadMount(vthread):
	/* Virtual thread is being mounted but it has been suspended.
	 * This should happen after the virtual thread object is stored in targetThread->threadObject.
	 */
	if (1 == vthread.isSuspendedInternal) {
		Set the halt flag
	}


VirtualThreadUnmount(vthread):
	/* Carrier thread is being mounted but but it has been suspended. 
	 * This should happen after vthread.carrierThread is stored in targetThread->threadObject,
	 * and targetThread->currentContinuation is set to NULL.
	 */
	if (1 == vthread.carrierThread.isSuspendedInternal) {
		Set the halt flag
	}

GetThreadState(thread):
	if (1 == thread.isSuspendedInternal) {
		Set the JVMTI_THREAD_STATE_SUSPENDED flag 
	} else {
		Remove the JVMTI_THREAD_STATE_SUSPENDED flag
	}

[dipak-bagadiya]

@babsingh the carrier thread object is passed to GetThreadState(thread) function
the check (1 == thread.isSuspendedInternal) will always check for carrier thread.

[babsingh]

@dipak-bagadiya The fix #16943 has been merged. Please re-enable the test for JDK20: ProblemList_openjdk20-openj9.txt#L540.

[babsingh]

Closing. The test has been re-enabled in adoptium/aqa-tests#4558.

[babsingh]

Re-opening the issue. The FIX is being reverted since it caused side-effects; see #17334 for more details.

PR to revert the FIX: #17335
PR to again exclude the test: adoptium/aqa-tests#4560

See #16943 (comment) for a potential fix, which might address the side-effects reported in #17334.

[babsingh]

@dipak-bagadiya The next steps are

• Re-open a new PR with the changes from #16943, add all new changes to address the recently seen side-effects and thoroughly review the code before testing.
• For testing, run a personal build with the following configurations:
  • JDK versions: 8, 11, 20
  • Test suites: sanity.functional, sanity.openjdk, extended.openjdk
• After the above steps, we will try to merge the changes again.

[dipak-bagadiya]

@babsingh Okay, will take the steps you've suggested and thoroughly review the code before testing.

[babsingh]

Closing.

• New fix: Fix to handle suspend/resume of virtual/carrier threads #17350.
• Test re-enabled: Re-enable JVMTI test WaitNotifySuspendedVThreadTest adoptium/aqa-tests#4575.