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

[Jaroslav Bachorik]

On 10/16/2014 02:14 AM, David Holmes wrote:
> 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.

Agreed. My concern was that the test code base would have been littered 
with `if (output == null) process.destroyForcible();` checks because 
there is no other way to react to the situation when process.waitFor() 
is interrupted - at least not in the JTreg context.

Therefore I put the logic of properly ending the external process to 
ProcessTools.executeProcess() method and restricted access to the 
constructors of OutputBuffer and OutputAnalyzer to enforce their 
creation only via ProcessTools.executeProcess().

Also, in order to prevent the started process stdout/stderr overflow I 
moved the backround stream pumpers to OutputBuffer so they would be 
started ASAP, without waiting for the process to exit (which defeats the 
purpose of consuming the attached stdout/stderr streams in backround 
anyway).

With these changes the API user doesn't need to worry about the external 
process cleanup anymore. The semantics of ProcessTools.executeProcess() 
guarantees that there will be no orphan process hanging about once this 
method returns.

This change is significantly bigger than the previous attempt because it 
spans a lot of tests using the OutputAnalyzer but, hopefully, it 
addresses David's concerns.

http://cr.openjdk.java.net/~jbachorik/8056143/webrev.01

-JB-

>
> Sorry.
>
> David
>
>> -JB-
>>
>>>
>>> David
>>>
>>>> -JB-
>>>>
>>>>
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -JB-
>>>>
>>