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

[Rob McKenna]

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