RFR 9: 8168517 : java/lang/ProcessBuilder/Basic.java failed

[Roger Riggs]

Hi David,

On 10/27/2016 9:00 PM, David Holmes wrote:
>>> But this is the second waitFor call after the process is destroyed.
>>> Sorry I don't really see the point.
>> The tests were added to determine if waitFor(timeout) was handling the
>> timeout parameter correctly.
>> The 2nd test here was to check the datapath when the process already
>> been destroyed.
>> The 'extra' waitFor() ensures the process is gone.
>> But you are correct, this exercises a path through the waitFor code that
>> does not even look
>> at the timeout value so I'll remove the entire 2nd case.
>>
>> Webrev updated in place.
>> http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/
>
> Okay, but now you don't need the p.waitFor() after the destroy() at all.
>
> Hmmm ... I guess either we want the original code with a longer 
> timeout to accommodate slow/loaded platforms (to test that waitFor(n) 
> after a destroy() is timely); or everything after destroy() can be 
> deleted.
Here's another approach to test whether waitFor(timeout) is behaving as 
expected.
The test should be checking that waitFor does not wait longer than 
requested.

So shorten the timeout to a very short time (10 milliseconds) and check that
waitFor returns in no more than that time(plus epsilon).  It is similar 
to the first test
in that section but the process is exiting.

    http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger


>
> Cheers,
> David
>
>
>> Thanks, Roger
>>> David
>>>
>>>>
>>>> Roger
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>> I contemplated increasing the timeout but given the issue is system
>>>>>> loading
>>>>>> I didn't have a good idea what to raise it to.  30sec, 1 min, 2 min.
>>>>>> and the harness timeout is 2min.
>>>>>>
>>>>>> Roger
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/
>>>>>>>>
>>>>>>>> Thanks, Roger
>>>>>>>>
>>>>>>
>>>>
>>