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