JEP 102 Process Updates revised API draft

[Roger Riggs]

Hi Peter,

Differing design approaches...

On 3/10/2015 12:37 PM, Peter Levart wrote:
> On 03/10/2015 10:50 AM, Paul Sandoz wrote:
>> On Mar 6, 2015, at 7:58 PM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
>>>> 2) I know there has been a lot of discussion about the use of CF,
>>>>      but I have a few more comments:
>>>>           a) Both onExit and onProcessExit are implemented to 
>>>> unconditionally
>>>>          throw UOE. Is the intention to make the implementation of 
>>>> these
>>>>          methods optional? If so, then UOE should be documented, If 
>>>> not,
>>>>          then I think they can be abstract, right?
>>> There are existing non-jdk implementations of Process and it would 
>>> be source
>>> incompatible if default implementations were not supplied. So, not 
>>> abstract.
>>>
>>> Since the behavior applies to Process, the note in the class 
>>> documentation
>>> defines the behavior.
>>> "Methods of {@code Process} that are not otherwise specified or 
>>> overridden throw
>>> {@link java.lang.UnsupportedOperationException}."
>>>
>>> More direct documentation could be provided in the override of onExit
>>> but that would raise the visibility to ordinary developers who 
>>> should use
>>> onProcessExit that returns the correctly typed CF and not onExit.
>>> The note should be sufficient for implementers without distracting 
>>> the ordinary developer.
>>>
>>> Perhaps, a description similar to that used in destroy(), the 
>>> behavior of Processes
>>> returned from ProcessBuilder can be more tightly specified. 
>>> Alternatively,
>>> ProcessBuilder could specify the behavior of Process instances it 
>>> returns, at present,
>>> the behavior of the Process instances is split between the two classes.
>>>
>> Throwing USO for default implementations is a warning sign to me that 
>> something does not fit quite right. It feels like we are giving up :-)
>
> I concur. I think current ProcessHandle is a mix of specification and 
> partial implementation that does not help in any way to hypothetical 
> external implementations. It effectively disables any consistent 
> alternative implementations. Take for example the following method in 
> ProcessHandle:
>
>     public final ProcessHandle parent() {
>         return ProcessHandleImpl.parent(getPid());
>     }
ProcessHandle is not intended to be *externally* extended; as an 
interface or abstract class
it can not make *any* guarantees about the implementation.
No caller can depend on any of the behavior based on the compile time type.
With the changes proposed, the callers using ProcessHandle must be 
prepared to handle UOE
in addition to dealing with the normal absent or missing information 
from the OS.

We should not open up another hole like Process without fully understanding
the use cases and the requirements on extensibility.  There may be a 
middle ground
that leaves the door open but not to start with.
>
> There's no way an alternative implementation of ProcessHandle could be 
> created that returned alternative implementations using method 
> parent(). What if alternative implementation doesn't support getPid()?
I have not considered supporting alternate implementations a goal but 
not breaking existing ones is essential.
>
> I think ProcessHandle should be mostly a specification and only have 
> abstract methods. Only a method that does not depend on implementation 
> could be concrete. I think it could be an interface. A package-private 
> constructor does not help as there is a Process class that has a 
> public constructor already, so anyone can create arbitrary instances 
> of arbitrary subclass of Process which is-a ProcessHandle...
Most of the behavior of ProcessHandle (as is Process) *is* 
implementation specific due to the OS dependencies.
When every function bottoms out in OS behavior, the layers above are 
dependent too.
>
>>
>> I now see Process.getPid was previously added and it throws USO by 
>> default. So we might be stuck due to the tricky nature of this area.
>
> Unfortunately. Default methods (in publicly extendable abstract 
> classes and interfaces) can not provide new functionality. They can 
> just re-pack existing functionality. getPid() is new functionality and 
> so are all new methods of ProcessHandle that Process inherits from. So 
> we can not just simply delegate to our implementation which might be 
> incompatible (unsupported platform).
Yes, but to support source level compatibility; some implementation is 
needed; though it cannot provide the full functionality.
>
>>
>> Any sub-type of Process that does not override getPid will 
>> essentially result in that USO being propagated to many ProcessHandle 
>> methods that depend on the PID (parent, children, allChildren, info, 
>> compareTo). That effectively renders ProcessHandle almost useless for 
>> sub-types outside of our control that that not been updated.
>
> So here's some changes I propose to the tip of JDK-8046092-branch that 
> hopefully fix these issues:
>
> http://cr.openjdk.java.net/~plevart/jdk9-sandbox/JDK-8046092-branch/webrev.02/ 
>
>
> The changes are:
> - Process.getPid() is back to throwing UOE if unsupported
> - ProcessHandle is all-abstract interface (only allChildren() is a 
> default method, since it is just a utility independent of implementation)
I considered 'abstract' in the PH API to be a distraction unless the API 
was intended to be extensible.

> - ProcessHandle.compareTo() specifies that only same-implementations 
> of ProcessHandle(s) are mutually comparable otherwise 
> ClassCastException must be thrown
Yes, avoids the issue with UOE.
> - ProcessHandleImpl.onExit() and ProcessImpl.onProcessExit() propagate 
> error from waitForProcessExit0 native method as exceptional completion 
> of CompletableFuture. exitValue is set to -1 on UNIX in this case 
> (instead of 0). What's the exitStatus on Windows in this case? This 
> area needs more work, I think.
The exceptional case was removed because the waitForExit does not throw 
an exception.
The only exception from Process.waitFor is InterruptedException and 
since there might
be spurious exceptions it should be retried.

When waitForProcessExit0 returns the process is gone; and the CF can be 
completed.

> - Some tweaks to javadocs in a few places
>
>>
>> Is it common to sub-type Process? If so then perhaps Process should 
>> not extend ProcessHandle and instead there should be a way to view a 
>> Process as a ProcessHandle, whose default implementation is:
>>
>>    return ProcessHandle.of(getPid()); // throws USO if getPid does
>>
>> (java.lang.ProcessImpl could implement Processhandle thereby the 
>> implementation is "return this".)
>>
>> Thus once you have a ProcessHandle instance (from whatever source) 
>> it's possible to operate on it without fear of a USO.
>>
>>
>> A possible default implementation for Process.onProcessExit could be:
>>
>>    return CF.supplyAsync(() -> { this.waitFor(); return p}, 
>> cachedThreadPool);
>
> It could be, but this would encourage external implementations to not 
> override it and this implementation is not very efficient in terms of 
> thread stack sizes. We could use a default implementation that is 
> similar to Windows' ProcessImpl - only use waitFor() instead of 
> ProcessHandleImpl.waitForProcessExit0() native method, but on UNIX 
> waitFor() *is* implemented on top of this implementation, so I think 
> the best is to force the implementor to do it optimally from the 
> ground-up. Default methods should really be reserved for something 
> very trivial that is not only implementation independent, but also 
> equally efficient regardless of underlying implementation.
Without external implementations, the code moved up from ProcessImpl and 
ProcessHandleImpl classes
to the abstract class to avoid duplication in the subclasses.
If external implementations of PH were a factor, the duplication would 
be more reasonable.

Thanks, Roger