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

Rob McKenna rob.mckenna at oracle.com
Wed Apr 18 18:15:46 UTC 2012

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.MAXVALUE 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.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