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

[David Holmes]

On 18/11/2014 2:43 PM, Martin Buchholz wrote:
> 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.

I don't really see the point of adding the extra math, plus an extra 
call (the two arg wait will call the one arg version) for no actual gain.

If you want to add a fast exit path that's fine but the rest seems 
superfluous to me.

David

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