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

[Roger Riggs]

Please  avoid the cross package dependency on j.u.TimeUnit.
Keeping mind that in Java ME we have to subset the API to make it fit
and that forciblyDestroy is needed iit would be cleaner to avoid the 
dependency
unless there will be a form of the method that uses only a fixed time 
unit (milliseconds).

Roger

On 04/20/2012 12:09 PM, 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.
>
>