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

David Holmes david.holmes at oracle.com
Fri Oct 28 20:59:03 UTC 2016


On 29/10/2016 5:39 AM, Roger Riggs wrote:
> Hi,
>
> On 10/28/2016 3:14 PM, David Holmes wrote:
>> 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?
> calling exitValue was just additional debugging info; but the test would
> have failed the timeout condition later.
> So the test is still broken, because it is not checking the correct
> timeout behavior of waitFor(timeout) and instead
> is checking how long it takes the OS to destroy.  That's not interesting.
>> 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().
> Lengthening the time isn't interesting either because it is only testing
> that it returns
> when the process has exited, not whether the timeout is hit at the right
> time.

There is no real way to test timeouts. You can check you don't get early 
returns but you can't check that you return in a timely fashion. At 
least without knowing the execution environment.

Does waitFor implement it's own timed-waiting mechanism, or does it just 
delegate to an existing API (like wait() or park()) ? If the latter then 
that is where timeout testing would exist.

The earlier part of the test already verifies both that the timeout 
mechanism functions, and that it doesn't return early.

> So I guess, its time to remove the whole test case on a destroyed process.

Given destroy() is not guaranteed to work either ... testing waitFor in 
that context will always be somewhat probabilistic I think - and timeout 
testing just makes that more so.

Cheers,
David

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