On Thu, 7 Jul 2022 19:08:35 GMT, Roger Riggs <[email protected]> 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.
Hello Roger, the change looks OK to me. The `ProcessBuilder` file will need a
copyright year update.
test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java line 59:
> 57: * @requires (os.family == "linux" & !vm.musl)
> 58: * @summary file descriptor leak with ProcessBuilder.startPipeline
> 59: * @run testng PipelineLeaksFD
Should this use `othervm` to avoid any kind of interference while we are
counting the file descriptors in this test?
test/jdk/java/lang/ProcessBuilder/PipelineLeaksFD.java line 101:
> 99:
> 100: Set<PipeRecord> pipesAfter = myPipes();
> 101: if (!pipesBefore.equals(pipesAfter)) {
Since `myPipes()` can return null, should there be a null check here for
`pipesBefore`?
-------------
PR: https://git.openjdk.org/jdk/pull/9414
