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

[Rob McKenna]

Hi folks,

The latest version is at:

http://cr.openjdk.java.net/~robm/4244896/webrev.03/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>

Feedback greatly appreciated.

     -Rob

On 19/04/12 12:05, Alan Bateman wrote:
> On 19/04/2012 01:05, David Holmes wrote:
>> On 18/04/2012 11:44 PM, 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?
>>
>> Also waitFor doesn't call wait() under the guard of a looping 
>> predicate so it will suffer from lost signals and potentially 
>> spurious wakeups. I also don't see anything calling notify[All] to 
>> indicate the process has now terminated. It would appear that 
>> wait(timeout) is being used as a sleep mechanism and that is wrong on 
>> a number of levels.
> I assume waitFor(timout) will require 3 distinct implementations, one 
> for Solaris/Linux/Mac, another for Windows, and a default 
> implementations for Process implementations that exist outside of the 
> JDK.
>
> It's likely the Solaris/Linux/Mac implementation will involve two 
> threads, one to block in waitpid and the other to interrupt it via a 
> signal if the timeout elapses before the child terminates. The Windows 
> implementation should be trivial because it can be a timed wait.
>
> I assume the default implementation (which is what is being discussed 
> here) will need to loop calling exitValue until the timeout elapses or 
> the child terminates. Not very efficient but at least it won't be used 
> when when creating Processes via Runtime.exec or ProcessBuilder.
>
> I think the question we need to consider is whether waitFor(timeout) 
> is really needed. If it's something that it pushed out for another day 
> then it brings up the question as to whether to include isAlive now or 
> not (as waitFor without timeout gives us an isAlive equivalent too).
>
> -Alan.
>
>
>
>