8373643: Test serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java still failing#29102
8373643: Test serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java still failing#29102sspitsyn wants to merge 3 commits intoopenjdk:masterfrom
Conversation
…ThreadListStackTracesTest.java still failing
|
👋 Welcome back sspitsyn! A progress list of the required criteria for merging this PR into |
|
@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 40 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 |
Webrevs
|
| } | ||
|
|
||
| public void ensureReadyAndWaiting(Thread vt, Thread.State expState, ReentrantLock rlock) { | ||
| sleep(50); // reliability: wait for a potential ReentrantLock class loading to complete |
There was a problem hiding this comment.
It is not clear how ReentrantLock might be not loaded already. Can you please explain what do you mean?
There was a problem hiding this comment.
There can be more than one class. E.g., some can be involved on contention.
There was a problem hiding this comment.
I've update the comment an removed the word ReentrantLock to avoid possible confusions.
| expState, state, jvmtiExpState, singleState, multiState); | ||
|
|
||
| if (vt.getState() != expState) { | ||
| if (state != expState) { |
There was a problem hiding this comment.
Assuming that there is no way to find if thread is completely locked, might be it makes a sense to just make a few attempts of checking status?
The test might sleep between attempts until got expected results. Even get them 3 times in a row.
So test fails only if we can't get to expected results during some reasonable time. So test would be more stable.
The only sleep between single check might be not enough in the case if VM/host is too busy.
There was a problem hiding this comment.
I did not want to go this way. My current goal is to make it clear the instability source is not on JVMTI side.
Also, I think even 3 time checking approach will give some failures.
|
This fix is a second correction of the test to make the output more clear. I wonder if it can be qualified as trivial. |
lmesnik
left a comment
There was a problem hiding this comment.
Thanks for updating comment. The fix can be considered as trivial.
|
Thank you for review, Leonid! |
|
/integrate |
|
Going to push as commit 12894a8.
Your commit was automatically rebased without conflicts. |
|
/backport :jdk26 |
|
@sspitsyn the backport was successfully created on the branch backport-sspitsyn-12894a87-jdk26 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk26, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk: |
The test is still failing after the fix of JDK-8371502. I suspect the issue is in the
ReentrantLockimplementation but suggest to make one more update of this test to make it more clear.The test update includes the following changes:
ensureReadyAndWaiting():sleep(50)at start of methodrlock.hasQueuedThreads()with call torlock.hasQueuedThread(vt)checkStates()to make it more stable and tracing output more clearTesting:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29102/head:pull/29102$ git checkout pull/29102Update a local copy of the PR:
$ git checkout pull/29102$ git pull https://git.openjdk.org/jdk.git pull/29102/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29102View PR using the GUI difftool:
$ git pr show -t 29102Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29102.diff
Using Webrev
Link to Webrev Comment