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

[Alan Bateman]

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.