On Wed, 17 Jul 2024 21:41:16 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Proposing a new overload method for `Process#waitFor()` which takes a
>> `Duration` for the timeout value. This will reduce the possibility for
>> making mistakes with the `TimeUnit` in the other overload method. A
>> corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ProcessTools overriding one-arg waitFor()
test/jdk/java/lang/Process/WaitForDuration.java line 57:
> 55: throws IOException, InterruptedException {
> 56: assertEquals(expected,
> 57: new ProcessBuilder("sleep", "3").start().waitFor(d));
I think in its current form, this has a chance of failure (for inputs like 0 or
negative duration), if the sleep (for 3 seconds) completes (and thus the
process exits) before the `Process.waitFor` implementation has had a chance to
execute `hasExited()`.
Also, this test is marked to run on all platforms. I think we might need
special handling for `sleep` executable on Windows. In fact, looking at the
`initSleepPath` in the `test/jdk/java/lang/ProcessBuilder/Basic.java` test, I
suspect we might need something similar in this test even for *nix.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20220#discussion_r1682143756
