8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context#8878
8286960: Test serviceability/jvmti/vthread/SuspendResume2 crashed: missing ThreadsListHandle in calling context#8878sspitsyn wants to merge 1 commit intoopenjdk:masterfrom
Conversation
…ssing ThreadsListHandle in calling context
|
👋 Welcome back sspitsyn! A progress list of the required criteria for merging this PR into |
dholmes-ora
left a comment
There was a problem hiding this comment.
These changes look good to me - as per our discussion in JBS issue.
Thanks,
David
|
@sspitsyn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 21 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
One query though - don't we have a self-suspension test that would have triggered the guarantee failure? If we don't then we should (obviously the resume will be a bit racy). |
|
The test |
|
David, thank you for review and help with analysis of this problem! |
|
Thank you for quick review, Patricio! |
|
Thank you for review, Alex! |
|
/integrate |
|
Going to push as commit 94811c0.
Your commit was automatically rebased without conflicts. |
|
I was expecting there to be an existing non-vthread-related test for self-suspension. That seems to be a hole in our test coverage. |
|
This nsk.jvmti test is also failing with the guarantee for current platform thread: Full list of nsk.jvmti tests that fail by this root cause: To make it clear, these tests did not fail without my fix because the TLH was protecting current thread being suspended. My fix made current thread unprotected by the TLH in |
|
Ah I see. Hmmm it might be argued (when Dan gets back) that the TLH was in fact intended/expected to cover the current thread - even though the current thread never needs such protection. |
|
Requiring protection of current thread with TLH makes it ugly for cases of self suspend as a nested extra TLH is required to protect current thread. The issue with self suspends is that having a jvmtiVTMSTransitionDisabler in its context is not allowed as it would cause deadlocks. |
|
Before Loom, there was a TLH in the outer JVM/TI SuspendThread() entry point so it was definitely my intent that the current thread be protected |
|
Yes, I understand it. Thank you for clarification! |
A part of this issue was contributed with the following changeset:
commit ea23e73
Author: Daniel D. Daugherty <dcubed@openjdk.org>
Date: Mon Nov 8 14:45:04 2021 +0000
The following change in
src/hotspot/share/runtime/thread.cppadded new assert:This new assert misses a check for target thread as being current
JavaThread.Also, the JVMTI SuspendThread is protected with TLH:
However, it is possible that a new carrier thread (and an associated
JavaThread) can be created after theTLHwas set and the target virtual thread can be mounted on new carrier thread. Then target virtual thread will be associated with newly createdJavaThreadwhich is unprotected by the TLH.The right way to be protected from this situation it is to prevent mount state transitions with
JvmtiVTMSTransitionDisablerbefore the TLH is set as in the change below:This problem exist in all JVMTI Suspend functions:
SuspendThread,SuspendThreadListandSuspendAllVirtualThreads.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8878/head:pull/8878$ git checkout pull/8878Update a local copy of the PR:
$ git checkout pull/8878$ git pull https://git.openjdk.java.net/jdk pull/8878/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8878View PR using the GUI difftool:
$ git pr show -t 8878Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8878.diff