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

[David Holmes]

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.

David

> Jason
>
>
>> Date: Tue, 17 Apr 2012 15:56:30 +0100
>> From: rob.mckenna at oracle.com
>> To: Alan.Bateman at oracle.com
>> Subject: Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
>> CC: core-libs-dev at openjdk.java.net
>>
>> New webrev at:
>>
>> http://cr.openjdk.java.net/~robm/4244896/webrev.01/
>>
>> Differences:
>> - implemented Process.waitFor(long timeout). I figured I'd throw it in
>> and let you decide if you want to keep it / replace isAlive.
>> - implemented defaults in Process.java
>> - destroyForcibly now returns the Process object
>> - Updated documentation
>> - Added @Override to all new overrides.
>> - Fixed long line in UNIXProcess_md.c consistent with other functions in
>> that file.
>> - Replaced WaitForSingleObject with GetExitCodeProcess: given the
>> concerns raised it seems to make more sense to leave users who return
>> STILL_ACTIVE as an error code on their own.
>> - Test updated to allow for proper execution of shell script.
>>
>> -Rob
>>
>> On 12/04/12 10:05, Alan Bateman wrote:
>>> On 12/04/2012 00:48, Rob McKenna wrote:
>>>> Hi folks,
>>>>
>>>> I'm hoping for some feedback on the above. Webrev:
>>>>
>>>> http://cr.openjdk.java.net/~robm/4244896/webrev.00/
>>> Thanks for taking this one on, a destroyForcibly is really useful to
>>> have (destroy was always misleading given that that is was actually a
>>> SIGTERM). The isAlive is also useful, another choice would be a
>>> waitFor that takes a timeout.
>>>
>>> As this patch adds two abstract methods it means there will be a
>>> source compatibility issue. Process has been in the platform since
>>> JDK1.0 so it likely there are other implementation, perhaps mocked.
>>> For destroyForcibly then a reasonable default could invoke the
>>> existing destroy but this would need to be specified. A possible
>>> default for isAlive is to invoke exitValue and mask the IAE when it
>>> hasn't terminated.
>>>
>>> On the javadoc then destroyForcibly needs to specify the return value,
>>> I think I missed this in the original patch that I gave you off-list.
>>> Having it return the Process is very useful as it allows for method
>>> invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also
>>> think destroyForcibly needs to set expectation that the process may
>>> not terminate immediately (isAlive might still return true for a brief
>>> period for example).
>>>
>>> The duplicate code in UNIXProcess.java.* is a reminder that we could
>>> combine the common code into something like a private package
>>> AbstractUNIXPorcess that would be extended by UNIXProcess, not for
>>> this patch of course.
>>>
>>> Looks like there is inconsistent use of @Override, you've added it to
>>> the isAlive implementation but not the others.
>>>
>>> Looks like UNIXProcess_md.c is straying beyond 80c so you might want
>>> to move the force parameter to the next line.
>>>
>>> Overall I think the implementation looks okay but one thing to think
>>> about is errors, WaitForSingleObject can fail with other errors, the
>>> return from kill is not checked.
>>>
>>> I don't have time to study the test is detail just now but I suspect
>>> invoking "./ProcessKillTest.sh" will need to change as the script will
>>> be in test.src and may not have execute permission. Also I see tests
>>> for specific exit codes which may be problematic (and will need to be
>>> updated for MacOSX). For isAlive then it may be better to test it by
>>> adding to existing tests.
>>>
>>> -Alan.
>>>
>>>
>