Re: JEP 102 Process Updates revised API draft

[Roger Riggs]

Hi,

The problem could be isolated to compareTo by defining the ordering if 
getPid
throws UOE and without diluting the spec for getPid returning the native 
os process identifier.

Defining the default for getPid() to return -1, might not have too big 
an impact.
It would order the incompletely implemented Process subtypes before the 
real ones
but the order is not usually significant except to be able to have a 
predictable iteration order
or use TreeMap. Returning Long.MAX_VALUE as the default might be another 
option.

Roger



On 3/9/2015 6:10 AM, Chris Hegarty wrote:
On 06/03/15 19:34, Jason Mehrens wrote:
Hi Chris,


Since getPid can throw UOE that means that compareTo could now throw 
UOE.

Ooh... I don't like this.

Has any consideration been given to getPid returning -1, if unknown or 
the default implementation? Or would this be any better?

-Chris




Jason


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