On Mon, 18 Jul 2022 17:53:16 GMT, Roger Riggs <rri...@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
