Hi Paul,
Comments inline:
On 11/05/12 12:29, Paul Sandoz wrote:
Hi Rob,
I dunno if the following has been pointed out before. It's hard to track review
comments.
The implementation of Process.waitFor normalizes the end time and duration
remaining time to nano seconds. However, the Thread.sleep method expects a
duration in units of milliseconds.
You could use:
http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#sleep(long,
int)
Or just round things up to the nearest millisecond:
Thread.sleep(Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));
That would be more consistent and wait for less time over that of the requested
duration.
Thanks for catching that.
For the following code:
227 long rem = end - now;
228 while (!hasExited&& (rem> 0)) {
229 wait(TimeUnit.NANOSECONDS.toMillis(rem));
230 rem = end - System.nanoTime();
231 }
Can the above go into a spin loop once the duration is less than 1 millisecond
for the rest of that duration? If so you may want to round it up to the nearest
millisecond.
Eesh. wait(0) will wait until notified so we could end up waiting past
our timeout expiry. Would:
wait(Math.max(TimeUnit.NANOSECONDS.toMillis(rem), 1));
alleviate all concerns here?
-Rob
Paul.
On May 10, 2012, at 8:56 PM, Rob McKenna wrote:
Hi folks,
The latest version is at:
http://cr.openjdk.java.net/~robm/4244896/webrev.03/<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>
Feedback greatly appreciated.
-Rob
On 19/04/12 12:05, Alan Bateman wrote:
On 19/04/2012 01:05, David Holmes wrote:
On 18/04/2012 11:44 PM, Jason Mehrens wrote:
Rob,
It looks like waitFor is calling Object.wait(long) without owning this objects
monitor. If I pass Long.MAX_VALUE to waitFor, shouldn't waitFor return if the
early if the process ends?
Also waitFor doesn't call wait() under the guard of a looping predicate so it
will suffer from lost signals and potentially spurious wakeups. I also don't
see anything calling notify[All] to indicate the process has now terminated. It
would appear that wait(timeout) is being used as a sleep mechanism and that is
wrong on a number of levels.
I assume waitFor(timout) will require 3 distinct implementations, one for
Solaris/Linux/Mac, another for Windows, and a default implementations for
Process implementations that exist outside of the JDK.
It's likely the Solaris/Linux/Mac implementation will involve two threads, one
to block in waitpid and the other to interrupt it via a signal if the timeout
elapses before the child terminates. The Windows implementation should be
trivial because it can be a timed wait.
I assume the default implementation (which is what is being discussed here)
will need to loop calling exitValue until the timeout elapses or the child
terminates. Not very efficient but at least it won't be used when when creating
Processes via Runtime.exec or ProcessBuilder.
I think the question we need to consider is whether waitFor(timeout) is really
needed. If it's something that it pushed out for another day then it brings up
the question as to whether to include isAlive now or not (as waitFor without
timeout gives us an isAlive equivalent too).
-Alan.
