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

[Rob McKenna]

Thanks a lot for the feedback folks.

Jason, as Alan notes, that makes a lot of sense. Thanks for that.

I'm planning to do the following over the next couple of days:

- fix UnixProcess.waitFor(long, TimeUnit) as per David's mail.
- alter the default waitFor(long, TimeUnit) comments/implementation as 
per Alan's comments.
- put some more work into firming up the test.

I'll get back to the alias with a new webrev when I'm done.

One thing I would note is that there is a "process reaper" thread in the 
UnixProcess implementations that will call Process.notifyAll() when the 
process exits.

     -Rob

On 20/04/12 17:09, Alan Bateman wrote:
> 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.
>
>