-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8290885: java/lang/ProcessBuilder/PipelineLeaksFD.java fail: More or fewer pipes than expected #9687
Conversation
…fewer pipes than expected Fix test so it does not leave unused stderr streams open
…gs/jdk into 8290885-Pipeline-leaks2
👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into |
|
@RogerRiggs The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
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.
Looks reasonable.
@RogerRiggs 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 |
for (int i = 0; i < builders.size() - 1; i++) { | ||
builders.get(i).redirectErrorStream(true); | ||
} |
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.
This loop could be removed by doing redirects in the data provider.
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.
Thanks for the review.
The effect would be the same, though it would distribute the needed constraint to more places in the source and the descriptive comment would need to be duplicated.
/integrate |
Going to push as commit 226b8e6.
Your commit was automatically rebased without conflicts. |
@RogerRiggs Pushed as commit 226b8e6. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The test java/lang/ProcessBuilder/PipelineLeaksFD.java fails intermittently, usually associated with fastdebug and -Xcomp.
It reports extra file descriptors are open that not expected.
The test of pipelines did not explicitly use or close the stderr streams of each stream except the last.
The intermittent nature of the failure is due to the non-deterministic GC interactions that close the streams when they become un-referenced.
The solution in this case is to redirect the error stream to the stdout stream for each pipeline stage except the last.
The test is removed from the ProblemList-Xcomp.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9687/head:pull/9687
$ git checkout pull/9687
Update a local copy of the PR:
$ git checkout pull/9687
$ git pull https://git.openjdk.org/jdk pull/9687/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9687
View PR using the GUI difftool:
$ git pr show -t 9687
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9687.diff