review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Alan.Bateman at oracle.com
Thu Apr 12 09:05:26 UTC 2012
On 12/04/2012 00:48, Rob McKenna wrote:
> Hi folks,
> I'm hoping for some feedback on the above. Webrev:
Thanks for taking this one on, a destroyForcibly is really useful to
have (destroy was always misleading given that that is was actually a
SIGTERM). The isAlive is also useful, another choice would be a waitFor
that takes a timeout.
As this patch adds two abstract methods it means there will be a source
compatibility issue. Process has been in the platform since JDK1.0 so it
likely there are other implementation, perhaps mocked. For
destroyForcibly then a reasonable default could invoke the existing
destroy but this would need to be specified. A possible default for
isAlive is to invoke exitValue and mask the IAE when it hasn't terminated.
On the javadoc then destroyForcibly needs to specify the return value, I
think I missed this in the original patch that I gave you off-list.
Having it return the Process is very useful as it allows for method
invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also
think destroyForcibly needs to set expectation that the process may not
terminate immediately (isAlive might still return true for a brief
period for example).
The duplicate code in UNIXProcess.java.* is a reminder that we could
combine the common code into something like a private package
AbstractUNIXPorcess that would be extended by UNIXProcess, not for this
patch of course.
Looks like there is inconsistent use of @Override, you've added it to
the isAlive implementation but not the others.
Looks like UNIXProcess_md.c is straying beyond 80c so you might want to
move the force parameter to the next line.
Overall I think the implementation looks okay but one thing to think
about is errors, WaitForSingleObject can fail with other errors, the
return from kill is not checked.
I don't have time to study the test is detail just now but I suspect
invoking "./ProcessKillTest.sh" will need to change as the script will
be in test.src and may not have execute permission. Also I see tests for
specific exit codes which may be problematic (and will need to be
updated for MacOSX). For isAlive then it may be better to test it by
adding to existing tests.
More information about the core-libs-dev