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