RFR 8056143: interrupted java/lang/management/MemoryMXBean/LowMemoryTest.java leaves running process

[David Holmes]

On 15/10/2014 11:55 PM, Jaroslav Bachorik wrote:
> On 10/15/2014 10:11 AM, David Holmes wrote:
>> On 15/10/2014 5:50 PM, Jaroslav Bachorik wrote:
>>> On 10/15/2014 02:10 AM, David Holmes wrote:
>>>> On 14/10/2014 8:46 PM, Jaroslav Bachorik wrote:
>>>>> Please, review the following test change
>>>>>
>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8056143
>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8056143/webrev.00
>>>>>
>>>>> The method jdk.testlibrary.ProcessTools.getOutput(process) waits for
>>>>> the
>>>>> given process to finish (process.waitFor()) before grabbing its
>>>>> outputs.
>>>>> However, the code does not handle the process.waitFor() being
>>>>> interrupted correctly - it just goes ahead and tries to obtain the
>>>>> exit
>>>>> code which will fail and leave the tested process running.
>>>>>
>>>>> The correct way is to forcibly destroy the process when
>>>>> process.waitFor() is interrupted or throws ExecutionException to make
>>>>> sure the process has actually exited before checking its exit code.
>>>>
>>>> Why is this correct? What gives the thread calling getOutput the right
>>>> to terminate the target process just because that thread was
>>>> interrupted
>>>> while waiting? If the interrupting thread intended the interrupt to
>>>> mean
>>>> "forcibly terminate the process and interrupt all threads waiting on
>>>> it"
>>>> then that thread should be doing the termination _not_ the one that was
>>>> interrupted!
>>>
>>> Process.waitFor() gets interrupted by a thread unknown to the actual
>>> test case - probably the JTreg timeout thread. The interrupting thread
>>> doesn't know that it is supposed to destroy a process. Once JTreg can
>>> take care of cleaning up process tree upon exit this code wouldn't be
>>> needed.
>>>
>>> I was contemplating adding the check for "null" returned from
>>> ProcessTools.getOutput() and destroying the process inside the caller
>>> code - but this would have the same results as doing it in
>>> ProcessTools.getOutput() with the drawback of duplicating the same check
>>> everywhere ProcessTools.getOutput() would be used.
>>>
>>> A silent postcondition of ProcessTools.getOuptut() is that the target
>>> process has finished - and it holds for all the code paths except the
>>> InterruptedException handler.
>>
>> That doesn't mean it is up to getOutput to forcibly terminate the
>> process. Multi-process cancellation is tricky, and yes eventually jtreg
>> will handle it. But this seems the wrong place to handle it now. Part of
>> the flaw here is that getOutput should itself throw InterruptedException
>> so that the caller is forced to deal with this - instead it just
>> re-asserts the interrupt state. The caller has to be aware that the
>> thread can be interrupted and do something appropriate - which may mean
>> punting to its caller. This is akin to a thread catching
>> InterruptedException and calling System.exit - it simply is not its job
>> to make that kind of decision at that level!
>
> There is no other decision to make. Not as it is written today. You can
> call ProcessTools.getOutput() and check whether the result is null and
> then end the test process. There is no other sensible action. The
> Process.waitFor() was interrupted you have no data to perform the checks
> against so the test will fail and as such it should stop any external
> processes it has started.
>
> Yes, I can go through all the tests using ProcessTools.getOutput() and
> add `if (output == null) process.destroyForcible();` - would this make
> it a better solution than putting this logic inside
> ProcessTools.getOutput()?

It would be the correct solution. Hacking it into getOutput() is just a 
convenience. Problem is that none of these tests have given enough 
thought to the cancellation issue and general process management.

Sorry.

David

> -JB-
>
>>
>> David
>>
>>> -JB-
>>>
>>>
>>>
>>>>
>>>> David
>>>>
>>>>> Thanks,
>>>>>
>>>>> -JB-
>>>
>