Thanks a lot for the feedback folks.
Jason, as Alan notes, that makes a lot of sense. Thanks for that.
I'm planning to do the following over the next couple of days:
- fix UnixProcess.waitFor(long, TimeUnit) as per David's mail.
- alter the default waitFor(long, TimeUnit) comments/implementation as
per Alan's comments.
- put some more work into firming up the test.
I'll get back to the alias with a new webrev when I'm done.
One thing I would note is that there is a "process reaper" thread in the
UnixProcess implementations that will call Process.notifyAll() when the
process exits.
-Rob
On 20/04/12 17:09, Alan Bateman wrote:
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.