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.
