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

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8048193: [tests] Replace JPS and stdout based PID retrieval by Process.getPid()
From:       Jaroslav Bachorik <jaroslav.bachorik () oracle ! com>
Date:       2014-07-01 13:00:18
Message-ID: 53B2B0E2.9060904 () oracle ! com
[Download RAW message or body]

On 07/01/2014 02:14 PM, Staffan Larsen wrote:
> 
> On 1 jul 2014, at 14:09, Jaroslav Bachorik <jaroslav.bachorik@oracle.com> wrote:
> 
> > On 07/01/2014 11:37 AM, Staffan Larsen wrote:
> > > 
> > > On 1 jul 2014, at 11:05, Jaroslav Bachorik <jaroslav.bachorik@oracle.com
> > > <mailto:jaroslav.bachorik@oracle.com>> wrote:
> > > 
> > > > On 07/01/2014 10:54 AM, Staffan Larsen wrote:
> > > > > 
> > > > > On 1 jul 2014, at 10:29, Jaroslav Bachorik
> > > > > <jaroslav.bachorik@oracle.com <mailto:jaroslav.bachorik@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@oracle.com
> > > > > > > <mailto:jaroslav.bachorik@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-
> > > 
> > 
> 


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