RFR 9: 8077350 Process API Updates Implementation Review (Due 4/23)

[Roger Riggs]

Hi Paul,

On 4/20/2015 2:26 PM, Paul Sandoz wrote:
> On Apr 20, 2015, at 7:33 PM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
>
>> Hi Paul,
>>
>> There are statements in Process about the specified behavior of Processes
>> created by ProcessBuilder.  That's why I included them in the @implSpec clause.
>> If @implSpec is only for the specifics of the method itself then where
>> should the behavior of ProcessBuilder created instances be specified?
>>
> I think the @implNote you have on Process.onExit about Processes from ProcessBuilder being more efficient is fine, but that @implNote also appears to mix stuff that is @implSpec for the method itself.
I don't see anything in that @implNote that is spec; just informative.
>
> For Process.supportsNormalTermination i would presume the @implSpec should be similar to that of toHandle. I don't think there needs to be anything said about Process from ProcessBuilder on this method this as it should "naturally" conform to what is stated.
ok on the first part.
But somewhere, it needs to state that ProcessBuilder created Processes 
are overloaded
to return true/false; otherwise it would be assumed they also throw UOE.

>
> Perhaps you can state something in the class doc of Process and or ProcessBuilder about the behaviour of such Process instances? It would seem to flow from the general statement you added to Process about overriding.
It may be more a question about consistency of where things are specified.
The current spec for Process includes spec for ProcessBuilder created 
instances.
Most developers will read Process (but not ProcessBuilder) to find the 
details.
Only ProcessBuilder.start() says any specific about the created Process.

I'll see what I make sense.

Thanks, Roger

>
> Paul.
>
>
>> Thanks, Roger
>>
>>
>>
>> On 4/20/2015 12:33 PM, Paul Sandoz wrote:
>>> On Apr 20, 2015, at 5:49 PM, Roger Riggs <Roger.Riggs at Oracle.com> wrote:
>>>
>>>> Hi Paul,
>>>>
>>>> On 4/20/2015 9:01 AM, Paul Sandoz wrote:
>>>>> Hi Roger,
>>>>>
>>>>> I am not sure you have the @implSpec/@implNote quite correct on the new methods of Process.
>>>>>
>>>>> For example, for Process.toHandle i would expect something like:
>>>>>
>>>>>    ...
>>>>>
>>>>>    @implSpec
>>>>>    This implementation throws an instance of UnsupportedOperationException and
>>>>>    performs no other action.  Sub-types should override this method, ensuring that
>>>>>    calling methods (getPid etc.) of this class, that are not overridden, operate on the
>>>>>    returned ProcessHandle.
>>>>>
>>>>> The @implSpec should refer to the implementation in Process itself, and the @implNote cannot be used for any normative statements.
>>>> Thanks for the reminder and suggested text.  I updated the @implSpec clauses.
>>>>
>>> I spotted another one here:
>>>
>>>   281     /**
>>>   282      * Returns {@code true} if the implementation of {@link #destroy} is to
>>>   283      * normally terminate the process,
>>>   284      * Returns {@code false} if the implementation of {@code destroy}
>>>   285      * forcibly and immediately terminates the process.
>>>   286      *
>>>   287      * @implSpec
>>>   288      * Processes returned from ProcessBuilder support this operation to
>>>   289      * return true or false depending on the platform implementation.
>>>   290      *
>>>   291      * @return {@code true} if the implementation of {@link #destroy} is to
>>>   292      *         normally terminate the process;
>>>   293      *         otherwise, {@link #destroy} forcibly terminates the process
>>>   294      * @throws UnsupportedOperationException if the Process implementation
>>>   295      *         does not support this operation
>>>   296      * @since 1.9
>>>   297      */
>>>   298     public boolean supportsNormalTermination() {
>>>   299         throw new UnsupportedOperationException("supportsNormalTermination not supported for " + this.getClass().toString());
>>>   300     }
>>>
>>>
>>> The docs of Process.onExit might also require some tweaks. I dunno how much wiggle room there is with the current implementation, perhaps very little?
>>>
>>>
>>>   415     /**
>>>   416      * Returns a ProcessHandle for the Process.
>>>   417      *
>>>   418      * @implSpec
>>>   419      * This implementation throws an instance of UnsupportedOperationException
>>>   420      * and performs no other action.
>>> ...
>>>   446      * @implSpec
>>>   447      * The implementation of this method returns information about the process as:
>>>   448      * {@link #toHandle toHandle().info()}.
>>>
>>> Best to be consistent with either "This implementation ..." or "The implementation of this method ..." rather than a mix. I prefer the former as it is more concise. Up to you.
>>>
>>>
>>>> Some @implNotes describe the JDK implementation and some developers will
>>>> rely on the implementation specifics and to some degree define the expected behaviors.
>>> One way of thinking about this is that the developers writing the TCK tests will pay close attention to @implSpec and need not do so for @implNote.
>>>
>>>
>>>>> The document for Process.getPid (and similarly those methods depending on toHandle) could then be:
>>>>>
>>>>>    ...
>>>>>    @implSpec
>>>>>    This implementation returns a process id as follows:
>>>>>
>>>>>      toHandle().getPid();
>>>>>
>>>>>
>>>>> In this respect is there a need to say anything about the behaviour of a Process created by ProcessBuilder?
>>>> The ProcessBuilder produced subclasses of Process implement the spec
>>>> so no additional description is needed.
>>>>> It might be useful to have some general guidance for sub-types on the class doc of Process e.g. saying they only need to override toHandle but may override some or all dependent methods as appropriate.
>>>> It does not add much but I added a paragraph to the Process class javadoc.
>>>> There are not many subclasses of Process outside the JDK.
>>>>
>>> Ok.
>>>
>>> Paul.
>>>
>>>> The webrev[1] and javadoc[2] have been updated in place.
>>>>
>>>> Thanks, Roger
>>>>
>>>> [1] http://cr.openjdk.java.net/~rriggs/webrev-ph
>>>> [2] http://cr.openjdk.java.net/~rriggs/ph-apidraft/