Anything but allowing UOE to escape compareTo sounds good.
Apologies if I missed this in previous threads but, shouldn't ProcessHandle.compareTo compare hostname, startInstant, and then pid? Or assuming they are not comparable between hosts then just startInstant and pid. Jason ---------------------------------------- > Date: Mon, 9 Mar 2015 14:43:20 +0000 > From: chris.hega...@oracle.com > To: roger.ri...@oracle.com; jason_mehr...@hotmail.com > CC: core-libs-dev@openjdk.java.net > Subject: Re: JEP 102 Process Updates revised API draft > > On 09/03/15 14:28, Roger Riggs wrote: >> 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. > > Yes, that would work. > >> 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. > > That would probably be ok too, and then the UOE could be removed from > Process.getPid() too, right? Which solves that small API wart. > > -Chris. > >> 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. >>>>> >>>>> >>>>> >>
