JEP 102 Process Updates revised API draft

[Roger Riggs]

Hi Peter,

On 3/10/2015 2:57 PM, Peter Levart wrote:
> Hi Roger,
>
> Thanks for bearing with me...
And me, as I find the route among the language, type system, 
compatibility issues,
and os intricacies.
>
> 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?
I'm reluctant because we're having to speculate about the details of 
what might be
with no one to speak directly to any particular target OS issues and not 
much evidence that it is needed.

>
>>
>> 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.
Perhaps
>
> UOE from getPid() is just a form of absent information. It could be -1.
This choice goes to the design of the caller;  is it structured to 
handle exceptions
or willing to check the validity of the values.
Many have complained that code to handle exceptions makes less code 
unreadable
and should be avoided.
>
>>
>> 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.
The argument is frequently made about making the surface area of the API 
larger
without taking into account who pays the cost and who gets the benefit.
>
>>>
>>> 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.
True for existing API; but then the salient discussion is about the 
semantics of the new functions
and how they can be expected to adhere to the contract if extensibility 
is supported.

>
>>>
>>> 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.
That's the risk.  Can it be eliminated or avoided...   Is it a necessary 
unknown?
>
>>>
>>>>
>>>> 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.
Just the aforementioned risk to maintenance cost
>
>>>
>>>>
>>>> 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?
It kept the bulk of the implementation details out of the API and made 
them available
to the ProcessImpls.  I'm still working on the organization of the 
implementation; there should
be a clean split between ProcessImpl's that spawn processes and 
ProcessHandleImpl for the other functions.
>
>>
>>> - 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.
There are a variety of OS level possible error conditions but either the 
process still exists
in which case it should still wait; or it is done and the exitValue (or 
not-available value)
is returned.
>
>>
>>> - 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, exitValue() 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.
yes, on Unix, it provides synchronization between the CF completions.
On Windows, it is redundant since by the time that code is executed, the 
process is already known to be done.

Thanks, Roger