On Mon, 29 Sep 2025 20:42:38 GMT, Roger Riggs <[email protected]> wrote:
>> The teardown of a Process launched by `ProcessBuilder` includes the closing >> of streams and ensuring the termination of the process is the responsibility >> of the caller. The `Process.close()` method provides a clear and obvious way >> to ensure all the streams are closed and the process terminated. >> >> The try-with-resources statement is frequently used to open streams and >> ensure they are closed on exiting the block. By implementing >> `AutoClosable.close()` the completeness of closing the streams and process >> termination can be done by try-with-resources. >> >> The actions of the `close()` method are to close each stream and destroy the >> process if it has not terminated. > > 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 16 additional > commits since the last revision: > > - Merge branch 'master' into 8364361-process-autocloseable > - Address review comments for code and javadoc in Process, the test and the > example. > Reinforced that calling waitFor should occur before calling close to wait > for termination and get the exitStatus. > Corrected the error message check for invalid handles on Windows. > Update test to allow normal completion when exceptions may be expected. > Correct the expected message on Windows for an invalid handle exception. > - Update close() to use "terminate" consistently. > - Remove volatile from "closed" field; updates are guarded by synchronized. > - The CSR review prompted the expansion of the description of > responsibilities > of subclasses of Process to invoke `super.close()` if overriding the > `close()` method. > - Monor improvements to javadoc, move close() near the constructor > to make it more prominent in the javadoc. > - Merge branch 'master' into 8364361-process-autocloseable > - Expand the description of close() to describe the behavior of destroying > the process. > - javadoc tweaks > - Misc, javadoc cleanup > Simplified the Interrupt handling in > ProcessCloseTest.ProcessCommand.ExpectExit. > - ... and 6 more: https://git.openjdk.org/jdk/compare/2aa98b7e...5a0c5602 Still look good. A couple of nits test/jdk/java/lang/Process/ProcessCloseTest.java line 318: > 316: private static void stderrExpectPolo(Process p) { > 317: String line = readLine(p.getErrorStream()); > 318: Assertions.assertEquals("Polo", line, "Stderr Expected > Polo"); } Curly bracket needs a new line test/jdk/java/lang/Process/ProcessCloseTest.java line 571: > 569: /* > 570: * Invasive hack to force closing a FileDescriptor to throw an > exception on FileDescriptor.close(). > 571: * The output stream is pealed back to get the FileDescriptor > containing the fd or handle. output -> input ------------- PR Review: https://git.openjdk.org/jdk/pull/26649#pullrequestreview-3285786463 PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2392163950 PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2392167896
