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

[David Holmes]

The process reaper thread will be calling process.notifyAll() so this is 
not simply a sleep. In which case the correct form would be:

public synchronized boolean waitFor(long timeout, TimeUnit unit) {
    ...
    while (!hasExited) {
       wait(timeLeft);
       if (!hasExited) {
         timeleft = recalcTimeLeft(...);
       }
       ...
    }

but using wait/notify the recalculation of the timeleft to wait becomes 
burdensome. At this point - issues of j.u.c dependencies not 
withstanding - it becomes simpler to use eg CountDownLatch for the 
synchronization.

David
-----

On 20/04/2012 7:27 PM, David Holmes wrote:
> Correction:
>
> On 20/04/2012 7:15 PM, David Holmes wrote:
>> Rob,
>>
>> You can't use wait this way:
>>
>> 217 public synchronized boolean waitFor(long timeout, TimeUnit unit)
>> 218 throws InterruptedException {
>> 219 long millis = unit.toMillis(timeout);
>> 220 long nanos = unit.toNanos(timeout) % (millis * 1000000);
>> 221
>> 222 if (hasExited) return true;
>> 223 if (timeout <= 0) return false;
>> 224 wait(millis, (int)nanos);
>> 225 return hasExited;
>> 226 }
>>
>> If this is just causing a delay then use Thread.sleep() (but don't have
>> the method synchronized of course). If something is actually calling
>> notifyAll (I don't see it) then the above suffers from lost wakeups.
>
> Sorry - There's no lost wakeup.
>
> David
> -----
>
>> Either way a spurious wakeup means you will return earlier than expected.
>>
>> David
>> -----
>>
>> On 20/04/2012 11:33 AM, 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
>>>
>>> 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.
>>>>
>>>>
>>>>
>>>>