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

[Alan Bateman]

On 17/04/2012 15:56, Rob McKenna wrote:
> 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
I don't have time to do a detailed review just now but I did go through 
the changes to java.lang.Process as I think you mostly need feedback on 
the API at this point.

One minor thing is that you'll need to add @since 1.8 to the new methods.

I think destroyForcibly's javadoc will need to specify the default 
implementation (ie for the case that these methods aren't  overridden), 
maybe something like "The default implementation of this method invokes 
destroy and so is implementation dependent as to whether it terminates 
the process forcibly or not. Implementations that are strong encouraged 
to override this method.".

I'm in two minds on waitFor(timeout). It is clearly useful but a default 
implementation will be awkward and probably will require a loop that 
polls and sleeps. I'm also not sure that a return of -1 is a good idea 
as you you can't distinguish this from a sub-process that completes with 
an exit code of -1. Maybe it would be better to return a boolean, like 
the awaitTermination methods in j.u.c. The javadoc is also a bit unclear 
as it states "wait for a specified timeout" whereas it is only useful if 
it waits up to the given timeout. Assuming that a timeout of 0 can be 
used to test if the process is alive then maybe isAlive is no required.

-Alan.