Subject: Re: JEP 102 Process Updates revised API draft
From: chris.hega...@oracle.com
Date: Fri, 6 Mar 2015 11:59:28 +0000
To: roger.ri...@oracle.com
CC: core-libs-dev@openjdk.java.net
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?
2) I know there has been a lot of discussion about the use of CF,
but I have a few more comments:
a) Both onExist 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?
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 ]
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 ?
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 )
e) Maybe onProcessExit would benefit from an apiNote to indicate
that it is essentially an alternative to waitFor() ?
3) Should ProcessHandle.getPid declare that it can throw IOE?
Process.getPid declares UOE.
4) ProcessHandle.Info.user() ? owner() seems more appropriate, no?
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.
6) There are a couple of @since 1.9 tags missing from Process
supportsDestroyForcibly and onProcessExit
That’s all for now.
-Chris.