The process reaper thread will be calling process.notifyAll() so this is
not simply a sleep. In which case the correct form would be:
public synchronized boolean waitFor(long timeout, TimeUnit unit) {
...
while (!hasExited) {
wait(timeLeft);
if (!hasExited) {
timeleft = recalcTimeLeft(...);
}
...
}
but using wait/notify the recalculation of the timeleft to wait becomes
burdensome. At this point - issues of j.u.c dependencies not
withstanding - it becomes simpler to use eg CountDownLatch for the
synchronization.
David
-----
On 20/04/2012 7:27 PM, David Holmes wrote:
Correction:
On 20/04/2012 7:15 PM, David Holmes wrote:
Rob,
You can't use wait this way:
217 public synchronized boolean waitFor(long timeout, TimeUnit unit)
218 throws InterruptedException {
219 long millis = unit.toMillis(timeout);
220 long nanos = unit.toNanos(timeout) % (millis * 1000000);
221
222 if (hasExited) return true;
223 if (timeout <= 0) return false;
224 wait(millis, (int)nanos);
225 return hasExited;
226 }
If this is just causing a delay then use Thread.sleep() (but don't have
the method synchronized of course). If something is actually calling
notifyAll (I don't see it) then the above suffers from lost wakeups.
Sorry - There's no lost wakeup.
David
-----
Either way a spurious wakeup means you will return earlier than expected.
David
-----
On 20/04/2012 11:33 AM, 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
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.
