On Fri, 12 May 2023 02:14:00 GMT, Patricio Chilano Mateo
<pchilanom...@openjdk.org> wrote:
> The following patch fixes a bug introduced while refactoring the
> VirtualThreadStart/End events. Specifically, the code to delete the
> JvmtiThreadState of a terminating vthread was moved before we start the VTMS
> transition. That allowed said code to run concurrently with
> recompute_enabled() leading to different crashing modes. I wrote the detailed
> sequence of events leading to the crash in the bug comments.
>
> To fix it I moved the cleanup code back after the call to
> VTMS_unmount_begin(). Now, since the rebinding of the JvmtiThreadState to
> that of the carrier has to be done after this cleanup code is executed,
> otherwise we would delete the wrong JvmtiThreadState state, I had to add a
> boolean argument to VTMS_unmount_begin() to differentiate the last unmount
> call from the other ones. This is unfortunate since ideally
> VTMS_unmount_begin() would be oblivious to these two cases as with
> VTMS_mount_end() where we don't need to check if this is the first mount.
> I looked for other ways to solve it instead of the extra boolean argument but
> wasn't convinced. One way would be to have another
> JvmtiExport::cleanup_thread() that would handle this case. Another way which
> is very simple is to move the rebind_to_jvmti_thread_state_of() call to
> VTMS_unmount_end() instead. But that means during the transition the
> _jvmti_thread_state field of the carrier would be either null or that of the
> vthread, unlike today which is always that of the carrier during the
> transitions. I didn't want to change that behavior in this fix but I can also
> explore that route.
>
> I tested the patch with the reproducer I attached to the bug, plus I also run
> tiers1-3 in mach5.
>
> Thanks,
> Patricio
Thank you for taking care about this issue!
Yes, clearing the `JvmtiThreadState` of a virtual thread has to be done
while in transition as it provides a needed synchronization.
This makes it a little bit ugly but I hope it can be simplified again after
getting rid of the `rebind_to_jvmti_thread_state_of()` which is still on my
TODO list.
Thanks,
Serguei
src/hotspot/share/prims/jvmtiThreadState.cpp line 559:
> 557: VTMS_unmount_begin(vthread, /* last_unmount */ true);
> 558: if (thread->jvmti_thread_state() != nullptr) {
> 559: assert(thread->jvmti_thread_state()->is_virtual(), "wrong
> JvmtiThreadState");
We agreed with you to temporarily remove this assert as it triggers the bug:
[8308124](https://bugs.openjdk.org/browse/JDK-8308124) dynamic loading of a
JVMTI agent has a race with JvmtiThreadState cleanup
A fix of the [8308124](https://bugs.openjdk.org/browse/JDK-8308124) will add
this assert back.
-------------
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13949#pullrequestreview-1427539217
PR Review Comment: https://git.openjdk.org/jdk/pull/13949#discussion_r1194485663
