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.
