8289643: File descriptor leak with ProcessBuilder.startPipeline #9414
Conversation
|
👋 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. |
Webrevs
|
|
Hello Roger, the change looks OK to me. The |
| Stream<Path> s = Files.walk(path); | ||
| return s.filter(Files::isSymbolicLink) | ||
| .map(p -> { | ||
| try { | ||
| return new PipeRecord(p, Files.readSymbolicLink(p)); | ||
| } catch (IOException ioe) { | ||
| } | ||
| return new PipeRecord(p, null); | ||
| }) | ||
| .filter(p1 -> p1.link().toString().startsWith("pipe:")) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
Is this intentionally leaking the returned DirectoryStream from Files.walk to trigger the previous failure mode or should this be auto-closed (i.e. https://errorprone.info/bugpattern/StreamResourceLeak )?
| Stream<Path> s = Files.walk(path); | |
| return s.filter(Files::isSymbolicLink) | |
| .map(p -> { | |
| try { | |
| return new PipeRecord(p, Files.readSymbolicLink(p)); | |
| } catch (IOException ioe) { | |
| } | |
| return new PipeRecord(p, null); | |
| }) | |
| .filter(p1 -> p1.link().toString().startsWith("pipe:")) | |
| .collect(Collectors.toSet()); | |
| try (Stream<Path> s = Files.walk(path)) { | |
| return s.filter(Files::isSymbolicLink) | |
| .map(p -> { | |
| try { | |
| return new PipeRecord(p, Files.readSymbolicLink(p)); | |
| } catch (IOException ioe) { | |
| } | |
| return new PipeRecord(p, null); | |
| }) | |
| .filter(p1 -> p1.link().toString().startsWith("pipe:")) | |
| .collect(Collectors.toSet()); | |
| } |
There was a problem hiding this comment.
@schlosna An oversight, thanks for the reminder.
|
@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 82 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 |
|
Hello Roger, I just noticed that the failures reported in the GitHub actions job are genuine and related to this new test. 2 separate failures: and |
|
Thanks @jaikiran, I should have expected Files.walk to have to deal with changes to /proc. |
|
Another alternative is to use Files.find as it can be used to find the sym links in the tree, e.g. |
…es due to unexpected pipes
…es due to unexpected pipes
…rRiggs/jdk into 8289643-pipeline-start-leak
|
|
|
Hello Roger, the latest state of this PR looks fine to me. The GitHub actions job too has now passed. I have added just a couple of minor comments inline. |
|
/integrate |
|
Going to push as commit 620c8a0.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@RogerRiggs Pushed as commit 620c8a0. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The
ProcessBuilder.pipelineStart()implementation does not close all of the file descriptors it uses to create the pipeline of processes.The process calling
pipelineStart()is creating the pipes between the stages.As each process is launched, the file descriptor is inherited by the child process and
the child process
dups them to the respective stdin/stdout/stderr fd.These copies of inherited file descriptors are handled correctly.
Between the launching of each Process, the file descriptor for the read-side of the pipe for the output of the previous process is kept open (in the parent process invoking
pipelineStart). The file descriptor is correctly passed to the child and is dup'd to the stdin of the next process.However, the open file descriptor in the parent is not closed after it has been used as the input for the next Process.
The fix is to close the fd after it has been used as the input of the next process.
A new test verifies that after
pipelineStartis complete, the same file descriptors are open for Unix Pipes as before the test.The test only runs on Linux using the /proc//fd filesystem to identify open file descriptors.
The bug fix is in
ProcessBuilder.pipelineStartand is applicable to all platforms.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9414/head:pull/9414$ git checkout pull/9414Update a local copy of the PR:
$ git checkout pull/9414$ git pull https://git.openjdk.org/jdk pull/9414/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9414View PR using the GUI difftool:
$ git pr show -t 9414Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9414.diff