RFR 9: 8064932: test java/lang/ProcessBuilder/Basic.java: waitFor didn't take long enough

[David Holmes]

Hi Roger,

On 18/11/2014 8:02 AM, roger riggs wrote:
> Hi,
>
> The technique used in the Linux version of Process.waitFor() is applied to
> the Windows version.  The duration of the native wait is measured
> using nanoTime and  the wait is repeated as necessary.
> For most uses, some jitter is expected due to workload, clock
> resolution, etc.
>
> Webrev:
>    http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/

This looks good to me.

As can be seen here:

http://msdn.microsoft.com/en-us/library/ms687069%28VS.85%29.aspx

The win32 wait functions can return early.

> Thanks, Roger
>
> On 11/17/2014 2:54 PM, Martin Buchholz wrote:
>> Returning early is EVIL.

Yet every OS seems to allow it <sigh>

Cheers,
David


>> ("""What part of 'wait for NNN nanoseconds' did you not understand??""")
>> Unfortunately, Object.wait may do so.  And perhaps also
>> waitForMultipleObjects.
>> HIgher level libraries need to be paranoid and compensate.
>>
>> On Mon, Nov 17, 2014 at 11:27 AM, roger riggs <roger.riggs at oracle.com>
>> wrote:
>>> Hi,
>>>
>>> I need to go back and identify the platform of the failure;  it is
>>> failing
>>> on Windows
>>> so the correct code is in ProcessImpl_md.c in
>>> waitForTimeoutInterruptibly
>>> in which the timeout is in milliseconds.
>>>
>>> If waitForMultipleObjects can return 'early' then the same kind of loop
>>> used on Linux for testing nanoTime may be needed.
>>>
>>> Roger
>>>
>>>
>>>
>>>
>>> On 11/14/2014 7:38 PM, Martin Buchholz wrote:
>>>> Hi Roger,
>>>>
>>>> I keep staring at the code in UNIXProcess.java and am having trouble
>>>> imagining how waitFor could possibly return early - that loop can only
>>>> terminate when elapsed time as measured by System.nanoTime exceeds
>>>> timeoutAsNanos.  It's true that we might have truncation when
>>>> converting to millis, but then the wait should get called a second
>>>> time with arg 1 (which is suboptimal, yes - we should improve waitFor
>>>> (and very sadly, we should also improve Object.wait)).
>>>>
>>>> Is there a way for me to repro this test failure?  The JDK tries hard
>>>> to provide monotonic nanoTime().
>>>>
>>>> On Fri, Nov 14, 2014 at 2:15 PM, roger riggs <roger.riggs at oracle.com>
>>>> wrote:
>>>>> Hi Martin,
>>>>>
>>>>> The artifact is by-product of using System.nanoTime to measure the
>>>>> duration
>>>>> in UNIXProcess and the conversion to milliseconds to call
>>>>> Object.wait():
>>>>>
>>>>> The low order bits of nanoseconds are truncated in the conversion.
>>>>>
>>>>>           long timeoutAsNanos = unit.toNanos(timeout);
>>>>>           long startTime = System.nanoTime();
>>>>>           long rem = timeoutAsNanos;
>>>>>
>>>>>           while (!hasExited && (rem > 0)) {
>>>>>               wait(Math.max(TimeUnit.NANOSECONDS.toMillis(rem), 1));
>>>>>               rem = timeoutAsNanos - (System.nanoTime() - startTime);
>>>>>           }
>>>>>
>>>>> The issue may be further complicated by the test logic also doing the
>>>>> timing in nanoSeconds when the best Object.wait can do is
>>>>> milliseconds.
>>>>>
>>>>> If System.nanoTime is to be used, perhaps it should be modulo
>>>>> milliseconds.
>>>>>
>>>>> Roger
>>>>>
>>>>>
>>>>>
>>>>> On 11/14/2014 2:57 PM, Martin Buchholz wrote:
>>>>>> This sort of change may be a necessary concession to reality, but
>>>>>> before we go there ... as I said in a previous review, this test may
>>>>>> demonstrate a real bug in the jdk implementation.  Is this
>>>>>> system-dependent?  Is it easy to reproduce?  Can we fix the JDK?
>>>>>>
>>>>>> On Fri, Nov 14, 2014 at 11:48 AM, roger riggs
>>>>>> <roger.riggs at oracle.com>
>>>>>> wrote:
>>>>>>> Please review this test change to make the wait time in
>>>>>>> ProcessBuilder/Basic
>>>>>>> a bit more lenient.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/
>>>>>>>
>>>>>>> Issue:
>>>>>>>       8064932: java/lang/ProcessBuilder/Basic.java: waitFor
>>>>>>> didn't take
>>>>>>> long
>>>>>>> enough
>>>>>>>
>>>>>>> Thanks, Roger
>>>>>>>
>