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

[David Holmes]

Hi Roger,

On 19/11/2014 8:28 AM, roger riggs 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/

Can you add a comment before all

TimeUnit.NANOSECONDS.toMillis(remainingNanos + 999_999L)

to indicate it is rounding up the millis value.

Thanks,
David

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