RFR: 8289643: File descriptor leak with ProcessBuilder.startPipeline [v7]

[Jaikiran Pai]

On Mon, 18 Jul 2022 17:53:16 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> 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 `dup`s 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 `pipelineStart` is complete, the same file descriptors are open for Unix Pipes as before the test.
>> The test only runs on Linux using the /proc/<pid>/fd filesystem to identify open file descriptors.
>> 
>> The bug fix is in `ProcessBuilder.pipelineStart` and is applicable to all platforms.
>
> Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - Remove diagnostics output from ProcessBuilder
>  - Merge branch '8289643-pipeline-start-leak' of https://github.com/RogerRiggs/jdk into 8289643-pipeline-start-leak
>  - Add DEBUG diagnostics to determine cause of GitHub Action test failures due to unexpected pipes
>  - Merge branch 'master' into 8289643-pipeline-start-leak
>  - Add DEBUG diagnostics to determine cause of GitHub Action test failures due to unexpected pipes
>  - Use DirectoryStream and handle IOExceptions
>  - remove debug output
>  - Add TWR for Files.walk of /proc
>  - Cleanup of PipelineLeaksFD test improving error messages and source cleanup.
>  - 8289643: File descriptor leak with ProcessBuilder.startPipeline

Marked as reviewed by jpai (Reviewer).

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.

test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java line 43:

> 41:  * @requires (os.family == "linux" & !vm.musl)
> 42:  * @summary file descriptor leak with ProcessBuilder.startPipeline
> 43:  * @run testng/othervm -DDEBUG PipelineLeaksFD

It looks like the `DEBUG` system property is no longer used in this test. Should we remove this passing of this property too?

test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java line 119:

> 117:      * Collect a Set of pairs of /proc fd paths and the symbol links that are pipes.
> 118:      * @return A set of PipeRecords, possibly empty
> 119:      * @throws IOException if reading the directory entries of "/proc/<pid>/fd/*" fails

With these latest changes, it looks like the `IOException` will no longer be thrown by this method. Should we remove this from the javadoc then?

-------------

PR: https://git.openjdk.org/jdk/pull/9414