On Thu, 18 Jul 2024 05:11:12 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> ProcessTools overriding one-arg waitFor()
>
> src/java.base/share/classes/java/lang/Process.java line 501:
>
>> 499: if (hasExited())
>> 500: return true;
>> 501: if (duration.isZero() || duration.isNegative())
>
> Hello Naoto, I see that there's a `Duration.isPositive()` API. Should we use
> that here instead?
Good point. I forgot it although I introduced the methd back in JDK18 🙂
> 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.
Made the sleep length variable, and used one hour for zero or negative (should
be enough). For the positive, the process completes immediately. Also, I
changed the sleep part to pure Java so that it won't rely on the testing
platform.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20220#discussion_r1683292669
PR Review Comment: https://git.openjdk.org/jdk/pull/20220#discussion_r1683292765
