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

[Thomas Stüfe]

Hi, Rob,

src/solaris/native/java/lang/UNIXProcess_md.c:

You never check the return code of kill(2). kill() may fail if you
have no permission to kill the process (EPERM), or if the process id
is invalid, maybe it has already been reaped. In both cases an
exception would be nice, or if you decide this cannot happen in the
context of Process.java, at least an assert().

The same applies to GetExitCodeProcess() on windows.

Kind Regards,

Thomas Stuefe
SAP Germany

On Sun, Jun 24, 2012 at 2:57 PM, Rob McKenna <rob.mckenna at oracle.com> wrote:
> Hi folks,
>
> 5th, and hopefully final review has been posted to:
>
> http://cr.openjdk.java.net/~robm/4244896/webrev.05/
> <http://cr.openjdk.java.net/%7Erobm/4244896/webrev.05/>
>
> Let me know if there are any comments or concerns, and thanks a lot for the
> help so far.
>
>    -Rob
>
> On 01/06/12 01:41, David Holmes wrote:
>>
>> Hi Rob,
>>
>> This looks good to me. I'm glad to see that destroyForcibly mandates that
>> Process instances from ProcessBuilder.start and Runtime.exec must do a
>> forcible destroy. That addresses my concern about documenting the actual
>> implementations.
>>
>> Two minor comments:
>>
>> Process.java:
>>
>>  236      * {@link ProcessBuilder#start} and {@link Runtime#exec} are of
>> type that
>>
>> "are of type" -> "are of a type ..."
>>
>> ProcessKillTest.sh:
>>
>> BIT_FLAG is now unused.
>>
>> Thanks,
>> David
>>
>>
>> On 1/06/2012 1:05 AM, Rob McKenna wrote:
>>>
>>> Latest version including work on the spec language:
>>>
>>> http://cr.openjdk.java.net/~robm/4244896/webrev.04/
>>> <http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>
>>>
>>> -Rob
>>>
>>> On 10/05/12 19:56, Rob McKenna wrote:
>>>>
>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>>
>
>