Please avoid the cross package dependency on j.u.TimeUnit.
Keeping mind that in Java ME we have to subset the API to make it fit
and that forciblyDestroy is needed iit would be cleaner to avoid the
dependency
unless there will be a form of the method that uses only a fixed time
unit (milliseconds).
Roger
On 04/20/2012 12:09 PM, 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.
