New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8299777: Test runtime/NMT/BaselineWithParameter.java timed out #12663
Conversation
👋 Welcome back stefank! A progress list of the required criteria for merging this PR into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for finding and fixing this!
I have a list of hanging tests, which I will need to check now to see if they follow the same bad usage pattern - see https://bugs.openjdk.org/browse/JDK-8299555
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@stefank 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 9 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixup looks good. Good catch.
Thanks.
Thanks for the reviews! |
Going to push as commit fef1910.
Your commit was automatically rebased without conflicts. |
I found this issue while debugging a hang in runtime/NMT/JcmdSummaryClass.java. We have the same issue in a few other NMT tests, and the BaselineWithParameter.java is one of them.
The common pattern among these tests is that they have one main process that forks a jcmd process against itself. The forked jcmd process writes its output to stdout (and maybe stderr). The problematic part of these tests is that the main process doesn't drain the streams of the forked jcmd before it calls waitFor(). This usually works and the tests passes, and I guess that's because there's some internal buffer that the jcmd process can write to without blocking. However, if too much data gets written the process gets blocked and the test times out. I can reproduce this by artificially adding more output to the NMT jcmd with the following patch
This is the problematic test pattern:
The correct way to write these tests is simply to remove the waitFor call and let the OutputAnalyzer deal with waiting for the forked process to finish:
The secondary problem with this tests is that they forks the jcmd process twice, via two calls to pb.start(). This isn't causing the hangs, but we shouldn't be doing that.
I've tested this fix by running the tests in runtime/NMT with and without the nmtDcmd.cpp patch above.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12663/head:pull/12663
$ git checkout pull/12663
Update a local copy of the PR:
$ git checkout pull/12663
$ git pull https://git.openjdk.org/jdk pull/12663/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12663
View PR using the GUI difftool:
$ git pr show -t 12663
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12663.diff