8289643: File descriptor leak with ProcessBuilder.startPipeline

[rkmauryaa]

clean backport to fix file descriptor leak with ProcessBuilder.startPipeline. When using ProcessBuilder.startPipeline the pipes (or pipe if only 2 ProcessBuilder instance used) created are never closed.

with changes :-
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java
                                                         1     1     0     0   
==============================
TEST SUCCESS


without changes :-
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java
                                                         1     0     1     0 
==============================
TEST FAILURE

with expected output :-
test PipelineLeaksFD.checkForLeaks(java.util.ImmutableCollections$ListN@44d1a216): failure
java.lang.AssertionError: More or fewer pipes than expected
...

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8290885 needs maintainer approval
• JDK-8289643 needs maintainer approval

Issues

• JDK-8289643: File descriptor leak with ProcessBuilder.startPipeline (Bug - P4 - Approved)
• JDK-8290885: java/lang/ProcessBuilder/PipelineLeaksFD.java fail: More or fewer pipes than expected (Bug - P4 - Approved)

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/2462/head:pull/2462
$ git checkout pull/2462

Update a local copy of the PR:
$ git checkout pull/2462
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/2462/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2462

View PR using the GUI difftool:
$ git pr show -t 2462

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/2462.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @rkmauryaa, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user rkmauryaa" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

@rkmauryaa 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:

8289643: File descriptor leak with ProcessBuilder.startPipeline
8290885: java/lang/ProcessBuilder/PipelineLeaksFD.java fail: More or fewer pipes than expected

Reviewed-by: shade

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 29 new commits pushed to the master branch:

• 69e6736: 8314835: gtest wrappers should be marked as flagless
• 6c3a188: 8320681: [macos] Test tools/jpackage/macosx/MacAppStoreJlinkOptionsTest.java timed out on macOS
• 060c250: 8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
• 4071b8c: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls
• a640001: 8312194: test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java cannot handle empty modules
• a632247: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
• e00211b: 8321107: Add more test cases for JDK-8319372
• 6264c70: 8273153: Consolidate file_exists into os:file_exists
• 95f8dc0: 8316138: Add GlobalSign 2 TLS root certificates
• 0e524e2: 8324243: Compilation failures in java.desktop module with gcc 14
• ... and 19 more: https://git.openjdk.org/jdk17u-dev/compare/8f68f8f019bca842b4bdf366f3c364b25983fd9e...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[rkmauryaa]

/covered

[bridgekeeper]

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[openjdk]

⚠️ @rkmauryaa This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[mlbridge]

Webrevs

• 01: Full - Incremental (3d18ffad)
• 00: Full (0603c6ed)

[rkmauryaa]

/approval request Between the launching of each Process, the file descriptor for the output of the previous stage was being kept open, now we close that FD after it has been used as the input for the next Process. New regression test added as part of the backport fails without the fix, and passes with it (output is at the top of this thread in PR body).

[openjdk]

@rkmauryaa
8289643: The approval request has been created successfully.

[rkmauryaa]

/integrate

[openjdk]

@rkmauryaa
Your change (at version 0603c6e) is now ready to be sponsored by a Committer.

[rkmauryaa]

unclean backport of commit 226b8e680a82096b3d268703872d30182c10e326 which fixes intermittent failures observed for the newly added test. Changes in test/jdk/ProblemList-Xcomp.txt were causing conflicts but they are not needed with this new backport as addition and removal of problem-list entry is a no-op change and file contents remain unchanged.

Ran test again :

with changes :-
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java
                                                         1     1     0     0   
==============================
TEST SUCCESS


without changes :-
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java
                                                         1     0     1     0 
==============================
TEST FAILURE

with expected output :-
test PipelineLeaksFD.checkForLeaks(java.util.ImmutableCollections$ListN@44d1a216): failure
java.lang.AssertionError: More or fewer pipes than expected
...

[shipilev]

You also need to say /issue add JDK-8290885 to record this PR fixes that additional issue as well.

[rkmauryaa]

/issue add JDK-8290885

[openjdk]

@rkmauryaa
Adding additional issue to issue list: 8290885: java/lang/ProcessBuilder/PipelineLeaksFD.java fail: More or fewer pipes than expected.

[shipilev]

...and now also /approval 8290885 request <the fix request text goes here> for approval for that one.

[rkmauryaa]

/approval 8290885 request Backported commit 226b8e680a82096b3d268703872d30182c10e326 to fix intermittent failures of the newly added test caused because pipelines did not explicitly use or close the stderr streams of each stream except the last and relied on GC cycles which eventually closes them when they are not referenced.

[openjdk]

@rkmauryaa
8290885: The approval request has been created successfully.

[shipilev]

Looks fine. Wait for maintainer approval, and then I can sponsor.

[rkmauryaa]

/integrate

[openjdk]

@rkmauryaa
Your change (at version 3d18ffa) is now ready to be sponsored by a Committer.

[shipilev]

/sponsor

[openjdk]

Going to push as commit d1e31f6.
Since your change was applied there have been 29 commits pushed to the master branch:

• 69e6736: 8314835: gtest wrappers should be marked as flagless
• 6c3a188: 8320681: [macos] Test tools/jpackage/macosx/MacAppStoreJlinkOptionsTest.java timed out on macOS
• 060c250: 8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method
• 4071b8c: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls
• a640001: 8312194: test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java cannot handle empty modules
• a632247: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
• e00211b: 8321107: Add more test cases for JDK-8319372
• 6264c70: 8273153: Consolidate file_exists into os:file_exists
• 95f8dc0: 8316138: Add GlobalSign 2 TLS root certificates
• 0e524e2: 8324243: Compilation failures in java.desktop module with gcc 14
• ... and 19 more: https://git.openjdk.org/jdk17u-dev/compare/8f68f8f019bca842b4bdf366f3c364b25983fd9e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev @rkmauryaa Pushed as commit d1e31f6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.