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

[David Holmes]

Hi Roger,

On 28/10/2016 10:46 AM, Roger Riggs wrote:
> 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/

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.

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