On Sun, 28 Sep 2025 13:22:55 GMT, Jaikiran Pai <[email protected]> wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update close() to use "terminate" consistently.
>
> src/java.base/share/classes/java/lang/Process.java line 186:
>
>> 184: * <p>
>> 185: * Before calling this method the caller should read the streams
>> for any
>> 186: * data or text and call {@linkplain #waitFor() waitFor} if the
>> exit value is needed.
>
> Should we add some additional text which clearly highlights that waitFor()
> needs to be called (before close()) to allow the process to run to
> completion, even when the exit value or the stream contents aren't used/read
> by the calling application? I realize that a few lines above we specify:
>
>> The streams are closed immediately and the process is terminated without
>> waiting.
>
> But maybe explicitly saying that waitFor() is necessary for allowing the
> process to run to completion would help reduce confusion? When I first saw
> the PR, my initial thought was `Process.close()` would wait for the process
> to run to completion and cleanup the resources that were used and I suspect
> it might become a common pattern where applications do something like:
>
>
> try (Process p = new ProcessBuilder("some process that takes 5 seconds to
> complete normally").start()) {
> }
>
> and be surprised that the launched process was abruptly terminated. So maybe
> some clear text on this method would help?
I think those warnings are already present.
- `process is terminated without waiting`
- `the caller should read the streams for any data or text and call waitFor if
the exit value is needed`
- `Calling waitFor before calling close or exiting the try-with-resources block
allows the process time to clean up and exit.`
- and the example calls waitFor inside the try-with-resources.
Does this add the emphasis you are asking for:
`Calling waitFor after close() or after the try-with-resources block exits
returns the status after destroying the process.`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26649#discussion_r2388230174
