review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Alan Bateman]

On 12/04/2012 00:48, Rob McKenna wrote:
> Hi folks,
>
> I'm hoping for some feedback on the above. Webrev:
>
> http://cr.openjdk.java.net/~robm/4244896/webrev.00/
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.

-Alan.