On 1 jul 2014, at 14:09, Jaroslav Bachorik <[email protected]> wrote:
> On 07/01/2014 11:37 AM, Staffan Larsen wrote: >> >> On 1 jul 2014, at 11:05, Jaroslav Bachorik <[email protected] >> <mailto:[email protected]>> wrote: >> >>> On 07/01/2014 10:54 AM, Staffan Larsen wrote: >>>> >>>> On 1 jul 2014, at 10:29, Jaroslav Bachorik >>>> <[email protected] <mailto:[email protected]>> >>>> 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 >>>>>> <[email protected] >>>>>> <mailto:[email protected]>> 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- >> >
