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

David Holmes david.holmes at oracle.com
Fri Oct 28 19:14:36 UTC 2016


On 29/10/2016 3:48 AM, Roger Riggs wrote:
> 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/

But the first test is checking that we wait at-least as long as the 
timeout ie no early return when we know the process is still alive.

Sorry but this isn't making sense to me. The original problem is that 
waitFor(8) has timed out and we proceeded to call exitValue() while the 
process is still alive. If the process had in fact terminated then we 
would have hit the (end-start) check and failed due to a timeout. So 
what exactly are we trying to fix here? Do we simply prefer to fail due 
to timeout rather than have exitValue() throw IllegalStateException? If 
so then check for waitFor(8) timing out, but leave the timeout value at 
8 seconds - just avoid the call to exitValue() if a time-out occurred. 
Otherwise if we want to avoid timing out we either bump the timeout to 
some value > 8 to accommodate the load problem, or we don't even bother 
doing waitFor after the destroy().

I'm really unclear what this is trying to test, and what a failure would 
indicate.

Thanks,
David

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


More information about the core-libs-dev mailing list