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

[David Holmes]

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!

David

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