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

[Martin Buchholz]

The latest version looks good, but here are some more nitpicks:

I'd prefer the name "deadline" to "endTime" since we already use that
convention in j.u.c., e.g

final long deadline = System.nanoTime() + nanosTimeout;

With the "deadline" style of checking, variable remainingNanos becomes
unnecessary, and you can just do

do { ... } while (deadline - System.nanoTime() > 0)

On Tue, Nov 18, 2014 at 2:28 PM, roger riggs <roger.riggs at oracle.com> wrote:
> Hi,
>
> Done,  added the test and immediate return after the wait.
> On Windows, the Thread.interrupted check needs to be done first to be
> consistent with Unix.
> Refactored a bit to remove a local variable and extra arithmetic computing
> the remainingNanos.
>
> Updated:
> http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/
>
> Roger
>
>
>
>
> On 11/18/2014 4:58 PM, Martin Buchholz wrote:
>>
>> I think we're all 3 in agreement on the general direction, even if we
>> disagree on the details.
>>
>> Once we properly round to millis, it is no longer necessary to use
>> Math.max - just
>>
>> wait(NANOSECONDS.toMillis(rem + 999_999L));
>>
>> As in my proposed change, I would like every call to wait immediately
>> followed by a check
>> if (hasExited)
>>    return true;
>>
>> to avoid the extra call to nanoTime.
>>
>> On Tue, Nov 18, 2014 at 12:09 PM, roger riggs <roger.riggs at oracle.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> Work on Object.wait is an issue to be taken up separately.
>>>
>>> I agree that the timeout values should not be truncated to milliseconds,
>>> likely
>>> causing an additional cycle through the while loop and waiting.
>>> The Process.waitFor methods are expected to wait for the specified time
>>> to
>>> elapse.
>>> The webrev has been updated for both Windows and Unix to use a ceiling
>>> function
>>> on milliseconds and ensure that at least the requested time has elapsed.
>>>
>>> Please review:
>>>    http://cr.openjdk.java.net/~rriggs/webrev-basic-notenough-8064932/
>>>
>>> Thanks, Roger
>>>
>>>
>>>
>>>
>>> On 11/18/2014 11:08 AM, Martin Buchholz wrote:
>>>>
>>>> On Mon, Nov 17, 2014 at 8:54 PM, David Holmes <david.holmes at oracle.com>
>>>> wrote:
>>>>>
>>>>> 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.
>>>>
>>>> The idea was to code to the Object.wait API, not its current
>>>> implementation with only millisecond resolution.
>>>> Even if we code to the current implementation, we should round UP not
>>>> down,
>>>> to avoid the problem of needing to call wait twice in case of early
>>>> return.
>>>> That would also be progress.
>>>>
>>>>> If you want to add a fast exit path that's fine but the rest seems
>>>>> superfluous to me.
>>>
>>>
>