On Tue, 5 Aug 2025 18:21:24 GMT, Roger Riggs <rri...@openjdk.org> 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.
src/java.base/share/classes/java/lang/snippet-files/ProcessExamples.java line
38:
> 36: Paddling with the river flow;
> 37: Chilling still, go slow.
> 38: """;
Nit: I'm not against Haiku, though `writer.write("Hello, world!");` can be
enough to get the message across and save the reader (and the maintainer) 6 LoC.
src/java.base/share/classes/java/lang/snippet-files/ProcessExamples.java line
48:
> 46: // Read all lines and print each
> 47: List<String> lines = reader.readAllLines();
> 48: lines.forEach(System.err::println);
Simplification suggestion:
Suggestion:
reader.readAllLines().forEach(System.err::println);
Note that this will render the `j.u.List` import at the top redundant too.
src/java.base/share/classes/java/lang/snippet-files/ProcessExamples.java line
51:
> 49: var status = p.waitFor();
> 50: if (status != 0)
> 51: throw new RuntimeException("process status: " + status);
Suggestion:
throw new RuntimeException("unexpected process status: " +
status);
src/java.base/share/classes/java/lang/snippet-files/ProcessExamples.java line
53:
> 51: throw new RuntimeException("process status: " + status);
> 52: } catch (Throwable t) {
> 53: System.out.println("Process failed: " + t);
Shall we instead catch `Exception` here? Quoting from Effective Java:
> Do not catch `Throwable`. It is wrong to catch `Throwable`: it includes
> errors (such as `OutOfMemoryError`, `StackOverflowError`, etc.) that are not
> meant to be caught by applications.
Suggestion:
} catch (Exception e) {
System.out.println("Process failed: " + e);
test/jdk/java/lang/Process/ProcessCloseTest.java line 47:
> 45: * @bug 8336479
> 46: * @summary Tests for Process.close
> 47: * @library /test/lib
Unless I'm mistaken, there are no `/test/lib` dependencies:
Suggestion:
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2256334744
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2256356929
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2256342566
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2256350644
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2256365508
