[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8056143: interrupted java/lang/management/MemoryMXBean/LowMemoryTest.java
From:       David Holmes <david.holmes () oracle ! com>
Date:       2014-10-22 2:04:09
Message-ID: 54471099.9010507 () oracle ! com
[Download RAW message or body]

Sorry for the delay in getting back to this - I had a long weekend. :)

I think this new approach is great! So it is a big Thumbs Up from me!

Thanks,
David

On 17/10/2014 7:55 PM, Jaroslav Bachorik wrote:
> 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-
>>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]