review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid) (original) (raw)

David Holmes david.holmes at oracle.com
Thu Apr 19 00:05:52 UTC 2012

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.MAXVALUE 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.mckenna at oracle.com To: Alan.Bateman at oracle.com Subject: Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid) CC: core-libs-dev at 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 UNIXProcessmd.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 STILLACTIVE 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 UNIXProcessmd.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.

More information about the core-libs-dev mailing list