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

[Roger Riggs]

Hi David,

On 10/27/16 7:22 PM, David Holmes wrote:
> On 28/10/2016 6:58 AM, Roger Riggs wrote:
>> Hi David,
>>
>> On 10/27/2016 4:51 PM, David Holmes wrote:
>>>
>>>>> Won't that now cause the test to hang until timed-out by the harness?
>>>> yes, but an in-app timeout is not much different than the harness
>>>> timeout
>>>> so I didn't complicate the test. Under normal system loading it should
>>>> be 1-4 seconds.
>>>
>>> Okay, but then the logic following this change is redundant:
>>>
>>> 2407             int exitValue = p.waitFor();  // wait for it to be 
>>> done
>>> 2408
>>> 2409             start = System.nanoTime();
>>> 2410             p.waitFor(8, TimeUnit.SECONDS);
>>> 2411             end = System.nanoTime();
>>> 2412
>>> 2413             if ((end - start) > TimeUnit.SECONDS.toNanos(7))
>>> 2414                 fail("Test failed: waitFor took too long on a
>>> dead process. (" + (end - start) + "ns)"
>>> 2415                 + ", exitValue: " + exitValue);
>> Not entirely, it is intended to check that when waitFor is used with a
>> destroyed process it completes (pretty much) immediately.
>
> 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/

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