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

[Rob McKenna]

New webrev at:

http://cr.openjdk.java.net/~robm/4244896/webrev.01/

Differences:
- implemented Process.waitFor(long timeout). I figured I'd throw it in 
and let you decide if you want to keep it / replace isAlive.
- implemented defaults in Process.java
- destroyForcibly now returns the Process object
- Updated documentation
- Added @Override to all new overrides.
- Fixed long line in UNIXProcess_md.c consistent with other functions in 
that file.
- Replaced WaitForSingleObject with GetExitCodeProcess: given the 
concerns raised it seems to make more sense to leave users who return 
STILL_ACTIVE as an error code on their own.
- Test updated to allow for proper execution of shell script.

     -Rob

On 12/04/12 10:05, Alan Bateman wrote:
> 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.
>
>