JEP 102 Process Updates revised API draft

[Peter Levart]

Hi Roger,

Thanks for bearing with me...

On 03/10/2015 06:27 PM, Roger Riggs wrote:
>> 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

But Process is, and Process inherits methods from ProcessHandle which 
means that you are limiting external implementations of Process forever 
to status-quo functionality. Why not let the implementor of external 
Process implementation decide whether it needs or wants to implement 
this new functionality or not. Are you afraid that implementations will 
emerge, but will not be of enough quality because of impedance missmatch 
between the API and the target platform? Or are you afraid that it will 
be harder to support new additions to this API if there are external 
implementations created?

>
> 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.

So let's say the caller is on a platform that is not supported by JDK 
internal ProcessHandle implementation. What guarantees does 
ProcessHandle implementation give him? It think he's in better position 
if he must cope with differences than he is if he can't use the API at all.

UOE from getPid() is just a form of absent information. It could be -1.

>
> 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.

The hole is already open. Some external implementation of Process can 
already attempt to implement all the ProcessHandle instance API merely 
by subclassing the Process. Package-private constructor on ProcessHandle 
will not stop that. Final methods will though.

>>
>> 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.

Well, I don't see anything existing is broken if ProcessHandle doesn't 
impose final methods.

>>
>> 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.

So this is a question of: "Is ProcessHandle API suitable for other 
implementations or is it just for supported UNIX/Windows platforms", right?

Because technically it could be exposed as externally implementable with 
no backwards compatibility problems when 1st introduced. There might be 
problems later when extending it.

>>
>>>
>>> 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.

There's no source-level compatibility issue with ProcessHandle as it is 
new class or interface. Just with Process. And even with my changes, the 
source-level compatibility is maintained. Can you spot anything that is 
not source-compatible or does not behave like it used to? It actually 
behaves exactly like it behaves with your latest implementation when 
default Process.getPid() throws UOE. The only difference is that 
ProcessHandle is made extensible.

>>
>>>
>>> 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.

Well in that case, yes. But why is it ProcessHandle then split between 
ProcessHandle and ProcessHandleImpl? Isn't that a distraction too?

>
>> - 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.

So there's no error condition from waitForProcessExit0() then. Any 
return *is* exitValue(). I haven't checked the native implementations 
and just relied on comments in Java code.

>
>> - 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.

ProcesshandleImpl and Windows ProcessImpl can have same code, but UNIX 
ProcessImpl must additionally synchronize using waitFor() since on UNIX, 
extitValue() result is published by the continuation of completion() 
which can execute concurrently with asynchronous continuation of same 
completion() that completes user-visible CompletableFuture. There's no 
harm if synchronization using waitFor() is used in ProcesshandleImpl and 
Windows ProcessImpl too, but that's unneeded overhead.

Regards, Peter

> If external implementations of PH were a factor, the duplication would 
> be more reasonable.
>
> Thanks, Roger
>