Hi Rob,
A couple of minor things ...
Process.waitFor:
+ long startTime = System.nanoTime();
+ long rem = unit.toNanos(timeout) - (System.nanoTime() -
startTime);
should just be
+ long startTime = System.nanoTime();
+ long rem = unit.toNanos(timeout);
The second reading of nanoTime is not generally useful. Under ideal
conditions "no time" will have passed and you waste more time asking for
the updated time. Someone may argue that you could get preempted after
the first call to nanoTime and so we need to re-check; but you could get
preempted at any time: after the first nanoTime, after the second, after
the calculation of rem, etc. This is a game of "chase your tail" that
you can not win. These timeouts are only rough hints. Also you don't do
this double-read in UnixProcess.waitFor
And a style-nit: if(rem > 0), while(rem>0) - should have a space before
the (
---
UNIXProcess.java.linux
221 if (timeout <= 0) return false;
222
223 long timeoutAsNanos = unit.toNanos(timeout);
224 long startTime = System.nanoTime();
Wrong indentation.
---
UNIXProcess.java.solaris
167 if (timeout <= 0) return false;
168
169 long timeoutAsNanos = unit.toNanos(timeout);
Wrong indentation.
---
The test programs look okay. Time-based testing is always a potential
problem so we will just have to see how these go in practice.
David
-----
On 24/06/2012 10:57 PM, Rob McKenna wrote:
Hi folks,
5th, and hopefully final review has been posted to:
http://cr.openjdk.java.net/~robm/4244896/webrev.05/
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.05/>
Let me know if there are any comments or concerns, and thanks a lot for
the help so far.
-Rob
On 01/06/12 01:41, David Holmes wrote:
Hi Rob,
This looks good to me. I'm glad to see that destroyForcibly mandates
that Process instances from ProcessBuilder.start and Runtime.exec must
do a forcible destroy. That addresses my concern about documenting the
actual implementations.
Two minor comments:
Process.java:
236 * {@link ProcessBuilder#start} and {@link Runtime#exec} are of
type that
"are of type" -> "are of a type ..."
ProcessKillTest.sh:
BIT_FLAG is now unused.
Thanks,
David
On 1/06/2012 1:05 AM, Rob McKenna wrote:
Latest version including work on the spec language:
http://cr.openjdk.java.net/~robm/4244896/webrev.04/
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>
-Rob
On 10/05/12 19:56, 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.
