JEP 102 Process Updates revised API draft

[Roger Riggs]

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