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

[David Holmes]

On 28/10/2016 6:58 AM, Roger Riggs wrote:
> Hi David,
>
> On 10/27/2016 4:51 PM, David Holmes wrote:
>> On 28/10/2016 3:12 AM, Roger Riggs wrote:
>>> Hi David,
>>>
>>> On 10/27/2016 12:57 PM, David Holmes wrote:
>>>> Hi Roger,
>>>>
>>>> On 28/10/2016 1:44 AM, Roger Riggs wrote:
>>>>> Please review a test fix for a timeout on a busy system in
>>>>> Process.waitFor a destroyed process.
>>>>
>>>> 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.

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