RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()

[Jaroslav Bachorik]

On 07/01/2014 02:14 PM, Staffan Larsen wrote:
>
> On 1 jul 2014, at 14:09, Jaroslav Bachorik <jaroslav.bachorik at oracle.com> wrote:
>
>> On 07/01/2014 11:37 AM, Staffan Larsen wrote:
>>>
>>> On 1 jul 2014, at 11:05, Jaroslav Bachorik <jaroslav.bachorik at oracle.com
>>> <mailto:jaroslav.bachorik at oracle.com>> wrote:
>>>
>>>> On 07/01/2014 10:54 AM, Staffan Larsen wrote:
>>>>>
>>>>> On 1 jul 2014, at 10:29, Jaroslav Bachorik
>>>>> <jaroslav.bachorik at oracle.com <mailto:jaroslav.bachorik at oracle.com>>
>>>>> wrote:
>>>>>
>>>>>> On 07/01/2014 08:17 AM, Staffan Larsen wrote:
>>>>>>> Jaroslav,
>>>>>>>
>>>>>>> Great cleanup!
>>>>>>>
>>>>>>> How about using Process.destroyForcibly() instead of sending the
>>>>>>> “shutdown” message? Maybe not as “nice”, but much less code.
>>>>>>
>>>>>> The target process needs to hang around for a while till the test
>>>>>> tries to shut it down. We would need to put a monitor wait there and
>>>>>> rely on the OS being able to shut the process down.
>>>>>
>>>>> Not sure what you mean. Why can’t we terminate the process at the
>>>>> same place we call RunnerUtil.stopApplication()?
>>>>
>>>> The target application does nothing except of waiting to be shut down.
>>>> It needs to hang around for the test to be able to attach to it. It
>>>> used to listen on a socket to receive the shutdown message, now it
>>>> reads from stdin to block and wait for shutdown. If the read from
>>>> stdin is removed we would need to add another wait mechanism to make
>>>> sure the application is alive until the test shuts it down.
>>>
>>> Right. I would suggest a simple "for(;;) Thread.sleep(1000);” loop for this.
>>>
>>>>
>>>>>
>>>>>> The Process.destroyForcibly() spec leaves it to the OS specific
>>>>>> implementations to do the right thing. For the major OSes it seems
>>>>>> to force kill the process but I'm not sure about eg. embedded devices.
>>>>>
>>>>> I think the idea of Process.destroyForcibly() is that we /can/ rely
>>>>> on it. If we can’t rely on our own implementation, then the API is
>>>>> not very usable.
>>>>
>>>> Might be the case -
>>>> ...
>>>> * The default implementation of this method invokes {@link #destroy}
>>>> * and so may not forcibly terminate the process. Concrete implementations
>>>> * of this class are strongly encouraged to override this method with a
>>>> * compliant implementation.
>>>> ...
>>>>
>>>> If it can be confirmed that a process will always be forcibly
>>>> destroyed I have nothing against using this API call instead of
>>>> hand-crafting the shutdown mechanism.
>>>
>>> All of the implementation of Process in the JDK do provide methods for
>>> forcibly terminating the process. The above reference to a default
>>> implementation is for allowing other Process implementation outside of
>>> the JDK. Our testing would never encounter those.
>>
>> Yes, this sounds reasonable. Unfortunately, Process.destroyForcibly() sets the application's exit code to 143 and it makes a lot of the tests fail :(
>>
>> I would better leave it at checking the stdin for the shutdown message, even though it requires more code.
>
> OK. I just want us to consider using it for future tests.

Sure. Thanks!

-JB-

>
> /Staffan
>
>>
>> -JB-
>>
>>
>>
>>>
>>> /Staffan
>>>
>>>>
>>>> -JB-
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> test/sun/tools/jstatd/JstatdTest.java:
>>>>>>> 323                     port = Integer.toString(31111);
>>>>>>> //Utils.getFreePort());
>>>>>>> Looks like a mistake?
>>>>>>
>>>>>> Definitely a mistake :( Thanks for spotting it!
>>>>>>
>>>>>> Updated webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.01
>>>>>
>>>>> Looks good!
>>>>>
>>>>> Thanks,
>>>>> /Staffan
>>>>>
>>>>>
>>>>>>
>>>>>> -JB-
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> /Staffan
>>>>>>>
>>>>>>>
>>>>>>> On 30 jun 2014, at 18:43, Jaroslav Bachorik
>>>>>>> <jaroslav.bachorik at oracle.com
>>>>>>> <mailto:jaroslav.bachorik at oracle.com>> wrote:
>>>>>>>
>>>>>>>> Please, review the following test change.
>>>>>>>>
>>>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8048193
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8048193/webrev.00
>>>>>>>>
>>>>>>>> Intricate log parsing in order to get an application PID is
>>>>>>>> replaced with the new Process.getPid() API call. While doing this
>>>>>>>> cleanup it also become obvious that it was unnecessary to start a
>>>>>>>> socket server for each launched test application just in order to
>>>>>>>> shut it down when the same functionality can be achieved through
>>>>>>>> the usage of stdin/stdout provided by the Process instance.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> -JB-
>>>
>>
>