On 20/04/2012 02:33, Rob McKenna wrote:
I've uploaded another webrev to:
http://cr.openjdk.java.net/~robm/4244896/webrev.02/
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.02/>
I plan to spend some time over the coming day or two beefing up the
test for waitFor (right now its really geared towards destroyForcibly)
so I won't guarantee its 100% quite yet. That said, I would like
feedback on a couple of points before I proceed:
1) Alan suggested the use of System.nanoTime() so I altered
waitFor(long) to allow for a TimeUnit parameter. UnixProcess objects
can use Object.wait(long, int) but unfortunately
WaitForMultipleObjects (on Windows) only works to millisecond precision.
2) As Alan noted, there is really no need for isAlive() if people are
happy with the idea of waitFor(long, TimeUnit). I'd appreciate any
feedback on this aspect of the fix.
-Rob
Sorry I haven't time to reply before now.
I think destroyForcibly and isAlive are quite good. In destroyForcibly
then "invokes destroy" should probably be a link or @code. The @since
should be after the @return. For isAlive then I would suggest "Tests
whether the subprocess is live" rather an "Returns a boolean ...".
waitFor needs discussion. In 1 above you mentioned nanoTime but I think
I was actually suggesting we need to decide if the method is old-style
waitFor(timeout) or j.u.c/new-style waitFor(timeout,TimeUnit). As
Process doesn't have timeouts already then I don't see a problem
introducing the j.u.c style. I see you've also changed it to return a
boolean too so overall the signature looks much better. I did question
whether we need both isAlive and waitFor(0L,...) but Jason's comment
make sense, you don't want to have exception handling to poll a
sub-process. On the javadoc then I don't think it should mention that it
polls every 100ms, that's too closterfobic and someone will likely want
to test it too.
On the implementation then it looks like Windows is right but the
Solaris/Linux/Mac implementation doesn't wakeup if the sub-process
terminates before the timeout has elapsed. If this proves too difficult
then going forward with the destroyForcibly + isAlive, leaving waitFor
to another day is fine. On the default implementation (pre-jdk8 Process
implementations outside of the JDK) then I assume this method will
misbehave with a large timeout (the addition will overflow). You could
probably adjust the sleep timeout when the remaining time is <100ms.
-Alan.
