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

[Roger Riggs]

Hi Alan,

On 4/24/2015 8:06 AM, Alan Bateman wrote:
> On 17/04/2015 20:12, Roger Riggs wrote:
>> The webrev for ProcessAPI updates has been updated to reflect recent 
>> comments.
>> Please  review and comment by April 23rd.
>>
>> The updates include:
>>  - Renaming Process/ProcessHandle supportsDestroyForcibly to 
>> supportsNormalTermination
>>    and updating related descriptions
>> - ProcessHandle.destroy/destroyForcible descriptions have more 
>> consistent descriptions
>> - ProcessHandle.destroy now returns ProcessHandle to enable fluent use.
>> - Corrected description of default implementation ProcessHandle.onExit
>>
>> The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/
>>
>> The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph
> I'm finally getting to this.
>
> One comment on terminology is that ProcessBuilder uses phrases like 
> "Starts a new process" whereas the changes to Process (and the new 
> ProcessHandle) have switched to "spawn". It's not a big deal but would 
> be good to have consistency.
Spawn seemed more consistent with underlying OS; but I can use starts a 
process.
>
> A typo at around L77 of Process, "iecr" has crept in the class 
> description.
will fix.
>
> I'm not sure about the @implSpec in 
> Process::supportsNormalTermination. Shouldn't that just specify that 
> the default implementation throws UOE. An @implNote could comment on 
> how ProcessBuilder works. Same comment on Process::toHandle.
>
There needs to be a specification for what ProcessBuilder does; 
@implNote cannot provide testable assertions.

If @implSpec is scoped to exactly the behavior of the specific method, then
the behavior of ProcessBuilder will need to be in the main method 
description
as it is done for the existing methods. For example,
"Processes returned from ProcessBuilder override the default 
implementation ..."

> In Process::destroy then the wording is adjusted to use the term 
> "normally terminated". The word "normally" doesn't seem right to me, 
> "gracefully" might work better. I'm tempted to suggest 
> supportsNormalTermination be renamed to supportsGracefulTermination if 
> you haven't discounted it already.
There have been a variety of comments on the name and function and 
suggestions.
I arrived at 'normal' because it had a clearer mapping to the underlying 
OS behavior.
The descriptions of SIGTERM vary, but usually include a description of 
it as the normal way to terminate a process.

>
> The parent is optional and maybe it would make sense for parent() to 
> return Optional<ProcessHandle>. The javadoc for this method, isAlive 
> too, mention "zombie state". I think that needs a bit of description 
> to generally explain what it means.
Will providing Optional make it easier to use?

Perhaps the mention of zombie should be removed; it is os specific; 
though the description of the state may be useful.
>
> This may have been discussed previously but totalCpuDuration might be 
> clearer if renamed to totalCpuTimeAsDuration. Also startInstant would 
> be clearer (to me anyway) as startTimeAsInstant.
I would have preferred not include the return type at all instead of 
making the method name longer.
>
> A passing comment on the webrev (still working through it) is that it 
> would be good to keep the line length on the javadoc somewhat 
> consistent in the file.
I can work on that.

Thanks, Roger

>
> -Alan
>