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

[Jaroslav Bachorik]

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.

-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-
>