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

[Martin Buchholz]

Proposed sibling change
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/UNIXProcess.waitFor/
- don't unconditionally call nanoTime when the wait ends
- use the millis/nanos form of Object.wait in case sub-millisecond
waits are ever supported.

On Mon, Nov 17, 2014 at 6:28 PM, Martin Buchholz <martinrb at google.com> wrote:
> I was staring at that old process code I wrote many years ago, and I
> think it can be improved.  I'll post a patch later.
>
> On Mon, Nov 17, 2014 at 2:02 PM, roger riggs <roger.riggs at oracle.com> 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/
>>
>> Thanks, Roger
>>
>>
>>
>>
>> On 11/17/2014 2:54 PM, Martin Buchholz wrote:
>>>
>>> Returning early is EVIL.
>>> ("""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
>>>>>>>>
>>