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

[Ivan Gerasimov]

Thank you Pavel!


On 3/14/19 2:02 PM, Pavel Rappo wrote:
> 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.
>
Not quite.  The deadline can surely become negative, which is alright.  
Later we only check the difference (deadline - System.nanoTime()).

Actually, the new code mimics what we have in PocessImpl for Unix and 
Windows.

With kind regards,
Ivan

>   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
>>
>

-- 
With kind regards,
Ivan Gerasimov