Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Thanks Thomas, As per Alan's mail, we're going to deal with this as a separate issue after we've discussed it. -Rob On 25/06/12 10:52, Alan Bateman wrote: On 25/06/2012 09:56, Thomas Stüfe wrote: Hi, Rob, src/solaris/native/java/lang/UNIXProcess_md.c: You never check the return code of kill(2). kill() may fail if you have no permission to kill the process (EPERM), or if the process id is invalid, maybe it has already been reaped. In both cases an exception would be nice, or if you decide this cannot happen in the context of Process.java, at least an assert(). One thing to add to this is that the destroy method has been there since jdk1.0 and is not defined or specified to throw any exceptions so we can't really change that now. From the history it doesn't look like the return from kill(2) has ever been checked. As it can only ne used to send a signal to processes created by the ProcessBuilder or Runtime.exec API (not arbitrary process) then it is unlikely to fail. Whether the new destroy method should specify an exception may require consideration and maybe a separate bug should be created to consider that. -Alan
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Hi, Rob,
src/solaris/native/java/lang/UNIXProcess_md.c:
You never check the return code of kill(2). kill() may fail if you
have no permission to kill the process (EPERM), or if the process id
is invalid, maybe it has already been reaped. In both cases an
exception would be nice, or if you decide this cannot happen in the
context of Process.java, at least an assert().
The same applies to GetExitCodeProcess() on windows.
Kind Regards,
Thomas Stuefe
SAP Germany
On Sun, Jun 24, 2012 at 2:57 PM, Rob McKenna rob.mcke...@oracle.com 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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On 25/06/2012 09:56, Thomas Stüfe wrote: Hi, Rob, src/solaris/native/java/lang/UNIXProcess_md.c: You never check the return code of kill(2). kill() may fail if you have no permission to kill the process (EPERM), or if the process id is invalid, maybe it has already been reaped. In both cases an exception would be nice, or if you decide this cannot happen in the context of Process.java, at least an assert(). One thing to add to this is that the destroy method has been there since jdk1.0 and is not defined or specified to throw any exceptions so we can't really change that now. From the history it doesn't look like the return from kill(2) has ever been checked. As it can only ne used to send a signal to processes created by the ProcessBuilder or Runtime.exec API (not arbitrary process) then it is unlikely to fail. Whether the new destroy method should specify an exception may require consideration and maybe a separate bug should be created to consider that. -Alan
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On 24/06/2012 13:57, 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 Overall I think this has worked out well. I agree with David on the needless calls to System.nanoTime in Process.waitFor although it's not a major issue given that the implementation in the JDK overrides this method. The test additions, both in coverage and implementation, are much cleaned now, and better test coverage too. A lot of the discussion here has been on the default implementation of waitFor that Process provides but it's not covered by the tests. I didn't notice this before but it would be good to include a basic test for this, just so that code is exercise. The simplest may be to create a concrete implementation of Process that delegates to a Process instance returned by ProcessBuilder.start. Assuming you don't override the waitFor implementation then it means you will be able to exercise the default waitFor rather than the platform implementation. -Alan.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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(rem0) - 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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On 31/05/2012 17:48, Rob McKenna wrote:
That link should be:
http://cr.openjdk.java.net/~robm/4244896/webrev.04/
-Rob
I'm happy the spec has come good too. One small suggestion is to tweak
this line:
{@code Process} objects returned by {@link ProcessBuilder#start} and
{@link Runtime#exec} are of type that overrides this method and so
invoking this method will forcibly terminatethe process.
to
Invoking this method on {@code Process} objects returned by {@link
ProcessBuilder#start} and {@link Runtime#exec} will forcibly terminate
the process.
Alsoin this sentence:
... is called, however this method may be chained to {@code waitFor()}
if needed.
may be clearer if you put This method may be ... as a separate sentence.
Otherwise I think it reads well.I hope to review the implementation and
test soon.
-Alan
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
That link should be: http://cr.openjdk.java.net/~robm/4244896/webrev.04/ -Rob On 31/05/12 16:05, 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.
Re : review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Hi.
185 public boolean waitFor(long timeout, TimeUnit unit)
186 throws InterruptedException {
187 long now = System.nanoTime();
188
189 long end = now +
190 (timeout = 0 ? 0 : TimeUnit.NANOSECONDS.convert(timeout,
unit));
191
192 if (end = 0) // overflow
193 end = Long.MAX_VALUE;
194
195 long rem = end - now;
196 do {
197 try {
198 exitValue();
199 return true;
200 } catch(IllegalThreadStateException ex) {
201 if(rem 0)
202 Thread.sleep(
203 Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1,
100));
204 }
205 rem = end - System.nanoTime();
206 } while(rem 0);
207 return false;
208 }
If System.nanoTime() is close to wrapping, this code would consider overflow
even for a not-so-large timeout,
and the wait could stop earlier than expected.
(Also now should rather be called startTime or so (since at some point it's
no longer current time).)
One could do as in ScheduledThreadPoolExecutor, and use an offset on
System.nanoTime().
Or, just remove lines 192-194 : it's not really a problem if end wraps, since
it should unwrap
when doing end - System.nanoTime() (supposing we don't spend centuries in the
method).
Or, only work with delta (also supposing we don't wait for centuries), never
explicitly considering
an end value, like this:
public boolean waitFor(long timeout, TimeUnit unit)
throws InterruptedException {
final long startNS = System.nanoTime();
final long timeoutNS = (timeout = 0 ? 0 : unit.toNanos(timeout));
long remNS = timeoutNS;
do {
try {
exitValue();
return true;
} catch(IllegalThreadStateException ex) {
if(remNS 0)
Thread.sleep(
Math.min(TimeUnit.NANOSECONDS.toMillis(remNS) + 1, 100));
}
remNS = timeoutNS - (System.nanoTime() - startNS);
} while(remNS 0);
return false;
}
-Jeff
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: Re : review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Jeff,
On 1/06/2012 10:19 AM, Jeff Hain wrote:
Hi.
185 public boolean waitFor(long timeout, TimeUnit unit)
186 throws InterruptedException {
187 long now = System.nanoTime();
188
189 long end = now +
190 (timeout= 0 ? 0 : TimeUnit.NANOSECONDS.convert(timeout, unit));
191
192 if (end= 0) // overflow
193 end = Long.MAX_VALUE;
194
195 long rem = end - now;
196 do {
197 try {
198 exitValue();
199 return true;
200 } catch(IllegalThreadStateException ex) {
201 if(rem 0)
202 Thread.sleep(
203 Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1,
100));
204 }
205 rem = end - System.nanoTime();
206 } while(rem 0);
207 return false;
208 }
If System.nanoTime() is close to wrapping, this code would consider overflow
even for a not-so-large timeout,
If System.nanoTime is close to wrapping then we have all sorts of
problems to worry about. But you are right. The way to handle this with
no overflow issues is to track the elapsed time (System.nanoTime() -
start) which will always give a positive result when less than 2^63
nanoseconds elapse. That then gets subtracted from the requested timeout
to give the rem value.
and the wait could stop earlier than expected.
(Also now should rather be called startTime or so (since at some point it's
no longer current time).)
One could do as in ScheduledThreadPoolExecutor, and use an offset on
System.nanoTime().
Aside: I don't see that in latest version of STPE.
Cheers,
David
-
Or, just remove lines 192-194 : it's not really a problem if end wraps, since
it should unwrap
when doing end - System.nanoTime() (supposing we don't spend centuries in the
method).
Or, only work with delta (also supposing we don't wait for centuries), never
explicitly considering
an end value, like this:
public boolean waitFor(long timeout, TimeUnit unit)
throws InterruptedException {
final long startNS = System.nanoTime();
final long timeoutNS = (timeout= 0 ? 0 : unit.toNanos(timeout));
long remNS = timeoutNS;
do {
try {
exitValue();
return true;
} catch(IllegalThreadStateException ex) {
if(remNS 0)
Thread.sleep(
Math.min(TimeUnit.NANOSECONDS.toMillis(remNS) + 1, 100));
}
remNS = timeoutNS - (System.nanoTime() - startNS);
} while(remNS 0);
return false;
}
-Jeff
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Hi Rob,
Your specification for Process.waitFor(long timeout, TimeUnit unit) has
a conflict. You presently say:
If the subprocess has already exited then this method returns
immediately with the value {@code true}
and you also say:
If the current thread:
has its interrupted status set on entry to this method; or ...
then {@link InterruptedException} is thrown and the current thread's
interrupted status is cleared.
It can't do both. The code implements the first statement. The IE will
only be thrown if the thread will actually invoke sleep(). I suggest:
+ * pIf the current thread:
+ * ul
+ * lihas its interrupted status set when it would wait; or
+ * liis {@linkplain Thread#interrupt interrupted} while waiting,
Also note that the documentation for:
public abstract int waitFor() throws InterruptedException;
should be updated to properly document its interruption response, in
line with what is said for the waitFor(timeout) method.
---
In UNIXProcess.java as noted already you haqve to deal with the fact
that TimeUnit conversion routines truncate rather than round. I agree
with your suggestion to use Math.max(TimeUnit.NANOSECONDS.toMillis(rem),
1). It's not worth splitting out into millis+nanos to use the other form
of Object.wait(), and that form simply rounds to a millis value anyway.
---
In ProcessImpl.java you should also apply suitable rounding rather than
using waitForTimeoutInterruptibly(handle, unit.toMillis(timeout));
Also, as destroy() now states that its behaviour is
implementation-dependent, the overriding destroy() should update its
spec to clearly state that it does forceful termination. H but that
is useless as we are not dealing with public types ... Okay there is a
slight problem here. You declared that destroy() has
implementation-specific behaviour but there are no public subclasses
that can clarify what actual behaviour is provided. In a similar vein,
Process.waitFor(timeout) states that subclasses should override it to
avoid the polling loop, and the implementations do, but they are
non-public and so again there is no documention that the implementation
is actually improved. I'm not sure what can be done about this. Perhaps
add further implementation notes in Process.java to clarify what the JDK
platform specific sub-class implementations so ???
---
In the test:
50 if ((isOS(sol) p.exitValue() != 9) ||
51 ((isOS(lin) || isOS(mac)) p.exitValue() !=
137))
How reliable are these exit values for each OS? And should we be
checking for mac or bsd, or both? Plus this test should fail if run
on an unknown OS to indicate the test needs updating.
83 if(isOS(sol) || isOS(lin))
84 test.killProc(false); // make sure the SIGTERM is
trapped
Where is the mac/bsd case? Should the platform specific parts be
factored out -it would make the overall logic clearer I think.
In ProcessKillTest.sh:
- should this be used for windows? It's unclear to me if the logic here
works given that we don't send a SIGTERM on windows.
- I'm unclear if OSX/BSD are being handled correctly here too. Is
Darwin always the right value to check??
- Regarding this fragment
64 if [ -f ${FILE_LOCATION}${FS}JDK64BIT -a ${OS} = SunOS ]
65 then
66 BIT_FLAG=`cat ${FILE_LOCATION}${FS}JDK64BIT`
I seem to recall a recent discussion that indicated that JDK64BIT never
exists and that this is some old construct that has fallen into disuse.
Cheers,
David
-
On 11/05/2012 4:56 AM, 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
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
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.
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On May 11, 2012, at 5:24 PM, Rob McKenna wrote:
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?
Yes, or you could use:
wait(long timeout, int nanos)
Paul.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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 * 100);
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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 * 100);
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.
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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 * 100);
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.
RE: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Rob, 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. Process.isAlive is similar to Future.isDone(). I think isAlive fills that need to have a simple query method that doesn't throw exceptions. Jason
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Hi Rob, 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'll take a look as soon as I have a chance but will be OOTO the rest of today. 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. Object.wait (like Thread.sleep) doesn't honour the nanosecond argument so there is no point trying to use it. David - 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.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Thanks David, I suspect Alan simply meant that it may be preferable to use System.nanoTime to measure the time elapsed in the default implementation as opposed to System.currentTimeMillis. I had forgotten about the lower resolution of the timer interrupt. I'll revert that aspect of the change. -Rob On 20/04/12 02:53, David Holmes wrote: Hi Rob, 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'll take a look as soon as I have a chance but will be OOTO the rest of today. 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. Object.wait (like Thread.sleep) doesn't honour the nanosecond argument so there is no point trying to use it. David - 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.
RE: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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? Jason Date: Tue, 17 Apr 2012 15:56:30 +0100 From: rob.mcke...@oracle.com To: alan.bate...@oracle.com Subject: Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid) CC: core-libs-dev@openjdk.java.net New webrev at: http://cr.openjdk.java.net/~robm/4244896/webrev.01/ Differences: - implemented Process.waitFor(long timeout). I figured I'd throw it in and let you decide if you want to keep it / replace isAlive. - implemented defaults in Process.java - destroyForcibly now returns the Process object - Updated documentation - Added @Override to all new overrides. - Fixed long line in UNIXProcess_md.c consistent with other functions in that file. - Replaced WaitForSingleObject with GetExitCodeProcess: given the concerns raised it seems to make more sense to leave users who return STILL_ACTIVE as an error code on their own. - Test updated to allow for proper execution of shell script. -Rob On 12/04/12 10:05, Alan Bateman wrote: On 12/04/2012 00:48, Rob McKenna wrote: Hi folks, I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ Thanks for taking this one on, a destroyForcibly is really useful to have (destroy was always misleading given that that is was actually a SIGTERM). The isAlive is also useful, another choice would be a waitFor that takes a timeout. As this patch adds two abstract methods it means there will be a source compatibility issue. Process has been in the platform since JDK1.0 so it likely there are other implementation, perhaps mocked. For destroyForcibly then a reasonable default could invoke the existing destroy but this would need to be specified. A possible default for isAlive is to invoke exitValue and mask the IAE when it hasn't terminated. On the javadoc then destroyForcibly needs to specify the return value, I think I missed this in the original patch that I gave you off-list. Having it return the Process is very useful as it allows for method invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also think destroyForcibly needs to set expectation that the process may not terminate immediately (isAlive might still return true for a brief period for example). The duplicate code in UNIXProcess.java.* is a reminder that we could combine the common code into something like a private package AbstractUNIXPorcess that would be extended by UNIXProcess, not for this patch of course. Looks like there is inconsistent use of @Override, you've added it to the isAlive implementation but not the others. Looks like UNIXProcess_md.c is straying beyond 80c so you might want to move the force parameter to the next line. Overall I think the implementation looks okay but one thing to think about is errors, WaitForSingleObject can fail with other errors, the return from kill is not checked. I don't have time to study the test is detail just now but I suspect invoking ./ProcessKillTest.sh will need to change as the script will be in test.src and may not have execute permission. Also I see tests for specific exit codes which may be problematic (and will need to be updated for MacOSX). For isAlive then it may be better to test it by adding to existing tests. -Alan.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Eesh, thanks for spotting that Jason. New webrev will be incoming soon. -Rob On 18/04/12 14:44, 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? Jason Date: Tue, 17 Apr 2012 15:56:30 +0100 From: rob.mcke...@oracle.com To: alan.bate...@oracle.com Subject: Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid) CC: core-libs-dev@openjdk.java.net New webrev at: http://cr.openjdk.java.net/~robm/4244896/webrev.01/ Differences: - implemented Process.waitFor(long timeout). I figured I'd throw it in and let you decide if you want to keep it / replace isAlive. - implemented defaults in Process.java - destroyForcibly now returns the Process object - Updated documentation - Added @Override to all new overrides. - Fixed long line in UNIXProcess_md.c consistent with other functions in that file. - Replaced WaitForSingleObject with GetExitCodeProcess: given the concerns raised it seems to make more sense to leave users who return STILL_ACTIVE as an error code on their own. - Test updated to allow for proper execution of shell script. -Rob On 12/04/12 10:05, Alan Bateman wrote: On 12/04/2012 00:48, Rob McKenna wrote: Hi folks, I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ Thanks for taking this one on, a destroyForcibly is really useful to have (destroy was always misleading given that that is was actually a SIGTERM). The isAlive is also useful, another choice would be a waitFor that takes a timeout. As this patch adds two abstract methods it means there will be a source compatibility issue. Process has been in the platform since JDK1.0 so it likely there are other implementation, perhaps mocked. For destroyForcibly then a reasonable default could invoke the existing destroy but this would need to be specified. A possible default for isAlive is to invoke exitValue and mask the IAE when it hasn't terminated. On the javadoc then destroyForcibly needs to specify the return value, I think I missed this in the original patch that I gave you off-list. Having it return the Process is very useful as it allows for method invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also think destroyForcibly needs to set expectation that the process may not terminate immediately (isAlive might still return true for a brief period for example). The duplicate code in UNIXProcess.java.* is a reminder that we could combine the common code into something like a private package AbstractUNIXPorcess that would be extended by UNIXProcess, not for this patch of course. Looks like there is inconsistent use of @Override, you've added it to the isAlive implementation but not the others. Looks like UNIXProcess_md.c is straying beyond 80c so you might want to move the force parameter to the next line. Overall I think the implementation looks okay but one thing to think about is errors, WaitForSingleObject can fail with other errors, the return from kill is not checked. I don't have time to study the test is detail just now but I suspect invoking ./ProcessKillTest.sh will need to change as the script will be in test.src and may not have execute permission. Also I see tests for specific exit codes which may be problematic (and will need to be updated for MacOSX). For isAlive then it may be better to test it by adding to existing tests. -Alan.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
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. David Jason Date: Tue, 17 Apr 2012 15:56:30 +0100 From: rob.mcke...@oracle.com To: alan.bate...@oracle.com Subject: Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid) CC: core-libs-dev@openjdk.java.net New webrev at: http://cr.openjdk.java.net/~robm/4244896/webrev.01/ Differences: - implemented Process.waitFor(long timeout). I figured I'd throw it in and let you decide if you want to keep it / replace isAlive. - implemented defaults in Process.java - destroyForcibly now returns the Process object - Updated documentation - Added @Override to all new overrides. - Fixed long line in UNIXProcess_md.c consistent with other functions in that file. - Replaced WaitForSingleObject with GetExitCodeProcess: given the concerns raised it seems to make more sense to leave users who return STILL_ACTIVE as an error code on their own. - Test updated to allow for proper execution of shell script. -Rob On 12/04/12 10:05, Alan Bateman wrote: On 12/04/2012 00:48, Rob McKenna wrote: Hi folks, I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ Thanks for taking this one on, a destroyForcibly is really useful to have (destroy was always misleading given that that is was actually a SIGTERM). The isAlive is also useful, another choice would be a waitFor that takes a timeout. As this patch adds two abstract methods it means there will be a source compatibility issue. Process has been in the platform since JDK1.0 so it likely there are other implementation, perhaps mocked. For destroyForcibly then a reasonable default could invoke the existing destroy but this would need to be specified. A possible default for isAlive is to invoke exitValue and mask the IAE when it hasn't terminated. On the javadoc then destroyForcibly needs to specify the return value, I think I missed this in the original patch that I gave you off-list. Having it return the Process is very useful as it allows for method invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also think destroyForcibly needs to set expectation that the process may not terminate immediately (isAlive might still return true for a brief period for example). The duplicate code in UNIXProcess.java.* is a reminder that we could combine the common code into something like a private package AbstractUNIXPorcess that would be extended by UNIXProcess, not for this patch of course. Looks like there is inconsistent use of @Override, you've added it to the isAlive implementation but not the others. Looks like UNIXProcess_md.c is straying beyond 80c so you might want to move the force parameter to the next line. Overall I think the implementation looks okay but one thing to think about is errors, WaitForSingleObject can fail with other errors, the return from kill is not checked. I don't have time to study the test is detail just now but I suspect invoking ./ProcessKillTest.sh will need to change as the script will be in test.src and may not have execute permission. Also I see tests for specific exit codes which may be problematic (and will need to be updated for MacOSX). For isAlive then it may be better to test it by adding to existing tests. -Alan.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
New webrev at: http://cr.openjdk.java.net/~robm/4244896/webrev.01/ Differences: - implemented Process.waitFor(long timeout). I figured I'd throw it in and let you decide if you want to keep it / replace isAlive. - implemented defaults in Process.java - destroyForcibly now returns the Process object - Updated documentation - Added @Override to all new overrides. - Fixed long line in UNIXProcess_md.c consistent with other functions in that file. - Replaced WaitForSingleObject with GetExitCodeProcess: given the concerns raised it seems to make more sense to leave users who return STILL_ACTIVE as an error code on their own. - Test updated to allow for proper execution of shell script. -Rob On 12/04/12 10:05, Alan Bateman wrote: On 12/04/2012 00:48, Rob McKenna wrote: Hi folks, I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ Thanks for taking this one on, a destroyForcibly is really useful to have (destroy was always misleading given that that is was actually a SIGTERM). The isAlive is also useful, another choice would be a waitFor that takes a timeout. As this patch adds two abstract methods it means there will be a source compatibility issue. Process has been in the platform since JDK1.0 so it likely there are other implementation, perhaps mocked. For destroyForcibly then a reasonable default could invoke the existing destroy but this would need to be specified. A possible default for isAlive is to invoke exitValue and mask the IAE when it hasn't terminated. On the javadoc then destroyForcibly needs to specify the return value, I think I missed this in the original patch that I gave you off-list. Having it return the Process is very useful as it allows for method invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also think destroyForcibly needs to set expectation that the process may not terminate immediately (isAlive might still return true for a brief period for example). The duplicate code in UNIXProcess.java.* is a reminder that we could combine the common code into something like a private package AbstractUNIXPorcess that would be extended by UNIXProcess, not for this patch of course. Looks like there is inconsistent use of @Override, you've added it to the isAlive implementation but not the others. Looks like UNIXProcess_md.c is straying beyond 80c so you might want to move the force parameter to the next line. Overall I think the implementation looks okay but one thing to think about is errors, WaitForSingleObject can fail with other errors, the return from kill is not checked. I don't have time to study the test is detail just now but I suspect invoking ./ProcessKillTest.sh will need to change as the script will be in test.src and may not have execute permission. Also I see tests for specific exit codes which may be problematic (and will need to be updated for MacOSX). For isAlive then it may be better to test it by adding to existing tests. -Alan.
Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
On 12/04/2012 00:48, Rob McKenna wrote: Hi folks, I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ Thanks for taking this one on, a destroyForcibly is really useful to have (destroy was always misleading given that that is was actually a SIGTERM). The isAlive is also useful, another choice would be a waitFor that takes a timeout. As this patch adds two abstract methods it means there will be a source compatibility issue. Process has been in the platform since JDK1.0 so it likely there are other implementation, perhaps mocked. For destroyForcibly then a reasonable default could invoke the existing destroy but this would need to be specified. A possible default for isAlive is to invoke exitValue and mask the IAE when it hasn't terminated. On the javadoc then destroyForcibly needs to specify the return value, I think I missed this in the original patch that I gave you off-list. Having it return the Process is very useful as it allows for method invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also think destroyForcibly needs to set expectation that the process may not terminate immediately (isAlive might still return true for a brief period for example). The duplicate code in UNIXProcess.java.* is a reminder that we could combine the common code into something like a private package AbstractUNIXPorcess that would be extended by UNIXProcess, not for this patch of course. Looks like there is inconsistent use of @Override, you've added it to the isAlive implementation but not the others. Looks like UNIXProcess_md.c is straying beyond 80c so you might want to move the force parameter to the next line. Overall I think the implementation looks okay but one thing to think about is errors, WaitForSingleObject can fail with other errors, the return from kill is not checked. I don't have time to study the test is detail just now but I suspect invoking ./ProcessKillTest.sh will need to change as the script will be in test.src and may not have execute permission. Also I see tests for specific exit codes which may be problematic (and will need to be updated for MacOSX). For isAlive then it may be better to test it by adding to existing tests. -Alan.
