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

[David Holmes]

Hi Rob,

Your specification for Process.waitFor(long timeout, TimeUnit unit) has 
a conflict. You presently say:

"If the subprocess has already exited then this method returns
immediately with the value {@code true}"

and you also say:

"If the current thread:
   has its interrupted status set on entry to this method; or ...
then {@link InterruptedException} is thrown and the current thread's
interrupted status is cleared."

It can't do both. The code implements the first statement. The IE will 
only be thrown if the thread will actually invoke sleep(). I suggest:

+  * <p>If the current thread:
+  * <ul>
+   * <li>has its interrupted status set when it would wait; or
+   * <li>is {@linkplain Thread#interrupt interrupted} while waiting,

Also note that the documentation for:

public abstract int waitFor() throws InterruptedException;

should be updated to properly document its interruption response, in 
line with what is said for the waitFor(timeout) method.
---

In UNIXProcess.java as noted already you haqve to deal with the fact 
that TimeUnit conversion routines truncate rather than round. I agree 
with your suggestion to use Math.max(TimeUnit.NANOSECONDS.toMillis(rem), 
1). It's not worth splitting out into millis+nanos to use the other form 
of Object.wait(), and that form simply rounds to a millis value anyway.

---

In ProcessImpl.java you should also apply suitable rounding rather than 
using  waitForTimeoutInterruptibly(handle, unit.toMillis(timeout));

Also, as destroy() now states that its behaviour is 
implementation-dependent, the overriding destroy() should update its 
spec to clearly state that it does forceful termination. Hmmmm but that 
is useless as we are not dealing with public types ... Okay there is a 
slight problem here. You declared that destroy() has 
implementation-specific behaviour but there are no public subclasses 
that can clarify what actual behaviour is provided. In a similar vein, 
Process.waitFor(timeout) states that subclasses should override it to 
avoid the polling loop, and the implementations do, but they are 
non-public and so again there is no documention that the implementation 
is actually improved. I'm not sure what can be done about this. Perhaps 
add further implementation notes in Process.java to clarify what the JDK 
platform specific sub-class implementations so ???

---

In the test:

  50             if ((isOS("sol") && p.exitValue() != 9) ||
   51                 ((isOS("lin") || isOS("mac")) && p.exitValue() != 
137))

How reliable are these exit values for each OS? And should we be 
checking for "mac" or "bsd", or both? Plus this test should fail if run 
on an unknown OS to indicate the test needs updating.

  83         if(isOS("sol") || isOS("lin"))
   84             test.killProc(false);   // make sure the SIGTERM is 
trapped

Where is the mac/bsd case? Should the platform specific parts be 
factored out -it would make the overall logic clearer I think.

In ProcessKillTest.sh:

- should this be used for windows? It's unclear to me if the logic here 
works given that we don't send a SIGTERM on windows.
- I'm unclear if OSX/BSD are being handled correctly here too. Is 
"Darwin" always the right value to check??
- Regarding this fragment

   64     if [ -f ${FILE_LOCATION}${FS}JDK64BIT -a ${OS} = "SunOS" ]
   65     then
   66         BIT_FLAG=`cat ${FILE_LOCATION}${FS}JDK64BIT`

I seem to recall a recent discussion that indicated that JDK64BIT never 
exists and that this is some old construct that has fallen into disuse.

Cheers,
David
-----

On 11/05/2012 4:56 AM, 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.
>>
>>
>>
>>