Hi Chris,
Thanks for the comments.
On 3/6/2015 6:59 AM, Chris Hegarty wrote:
Roger,
I’ve taken a look at these changes in the sandbox ( JDK-8046092-branch ).
Overall I welcome this addition.
Some comments, most of which I stuffed into a webrev based on your branch,
http://cr.openjdk.java.net/~chegar/process_comments/webrev/
1) ProcessHandle.compareTo() can drop the ClassCastException
Also, I think the comparison is the wrong way around. It should
be compare(this, other), rather than compare(other, this), right?
yes, thanks
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.
b) The wording in the spec talks about async functions and actions.
I think this may be not quite right. The intention is to support, as is
provided by CF, the ability to chain both sync and async tasks.
[ I suggested some wording in the webrev ]
Yes, thanks for the wording.
Note that the completion of the returned CF is always performed
asynchronously.
Any additional behaviors dependent on the CF can be async or sync.
c) Why the need for ISE if the process is the current process, and not
just return a CF that never completes? Do you consider this an
error situation that you want to notify, or consistency with other
parts of the API ?
Since a CF for the current process will never complete it is a
programmer error and should
be notified immediately. Though consistency might be desirable, it
would lead to a situation
that is very hard to debug without the understanding that a PH happens
to refer to the current process.
d) I wonder if onProcessExit should have a small API note, saying
that it is preferred over onExit, when you have a Process. Or
something to promote its use over onExit, or briefly explain its
existence. ( I know why it is there, but it may appear as duplication )
I'm open to suggestions; it might become a distraction. The API goes to
some pains
to make sure onExit has minimal visibility in Process.
e) Maybe onProcessExit would benefit from an apiNote to indicate
that it is essentially an alternative to waitFor() ?
Yes, that would be useful to point out the additional functionality and
alternative
programming model.
3) Should ProcessHandle.getPid declare that it can throw IOE?
Process.getPid declares UOE.
Process declares UOE because not all subclasses can be guaranteed to
implement it.
All other implementations of ProcessHandle will create PH instances only
when there is an (initially) valid value for the pid.
But I see your point about the contract of ProcessHandle;
Process cannot add exceptional behavior outside of the scope defined by PH.
4) ProcessHandle.Info.user() ? owner() seems more appropriate, no?
Most typical command line tools identify it as the 'user';
(It was owner in a previous iteration).
5) The description of info() talks about fields, when it is an interface.
I added some suggested rewording. Also, all methods now return
references, so -1 can be removed. Similar for the Info class description.
Thanks for the improvements.
6) There are a couple of @since 1.9 tags missing from Process
supportsDestroyForcibly and onProcessExit
ok
Thanks, Roger
That’s all for now.
-Chris.
