RFR 8220684 : Process.waitFor(long, TimeUnit) can return false for a process that exited within the timeout

[Pavel Rappo]

I have to look at this patch in more detail, however here's what jumped out at
me straight away:

    long deadline = System.nanoTime() + remainingNanos;

It seems like a possibility for an overflow. Documentation for System.nanoTime
has a special section on this:

    For example, to measure how long some code takes to execute:

       long startTime = System.nanoTime();
       // ... the code being measured ...
       long elapsedNanos = System.nanoTime() - startTime;

    To compare elapsed time against a timeout, use

       if (System.nanoTime() - startTime >= timeoutNanos) ...

    instead of

     if (System.nanoTime() >= startTime + timeoutNanos) ...

    because of the possibility of numerical overflow.

Is that of concern in this case?

> On 14 Mar 2019, at 19:49, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
> 
> Hello!
> 
> The default implementation of Process.waitFor(long, TimeUnit) does not check if the process has exited after the last portion of the timeout has expired.
> 
> JDK has two implementations of Process (for Unix and Windows) and they both override waitFor(), so it's not an issue for them.
> 
> Still, it is better to provide a more accurate default implementation.
> 
> I'm not quite certain the regression test needs to be included in the fix.  The test does demonstrate the issue with the unfixed JDK and passed Okay on all tested platforms in Mach5.  Yet, I suspect the test can still show false negative results, as there are no guaranties that even such simple application as `true` will finish in 100 ms.
> I can tag the test as @ignored with a comment, or simply remove it from the fix.
> 
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8220684
> WEBREV: http://cr.openjdk.java.net/~igerasim/8220684/00/webrev/
> 
> Thanks in advance for reviewing!
> 
> -- 
> With kind regards,
> Ivan Gerasimov
>