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