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 <[email protected]> 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
>
