RFR 8220684 : Process.waitFor(long, TimeUnit) can return false for a process that exited within the timeout
Ivan Gerasimov
ivan.gerasimov at oracle.com
Fri Mar 15 02:02:11 UTC 2019
Hi David!
On 3/14/19 5:28 PM, David Holmes wrote:
> Hi Ivan,
>
> On 15/03/2019 5:49 am, Ivan Gerasimov 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.
>
> Please clarify. There is always a race between detecting a timeout and
> the process actually terminating. If the process actually terminates
> while you're deciding to report a timeout that seems just an
> acceptable possibility. No matter what you do the process could
> terminate just after you decided to report the timeout.
>
>
Current code for waitFor(...) is
212 do {
213 try {
214 exitValue();
215 return true;
216 } catch(IllegalThreadStateException ex) {
217 if (rem > 0)
218 Thread.sleep(
219 Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));
220 }
221 rem = unit.toNanos(timeout) - (System.nanoTime() -
startTime);
222 } while (rem > 0);
223 return false;
So, if the loop has just processed the last 100 ms portion of the
timeout, it ends right away, without checking if the process has exited
during this period.
Not a big deal of course, but it is more accurate to check if the
process has exited at the *end* of the specified timeout, and not 100
milliseconds before the end.
With kind regards,
Ivan
>
> David
> -----
>
>> 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
More information about the core-libs-dev
mailing list