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

[Staffan Larsen]

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.

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