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

[Alan Bateman]

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.

A typo at around L77 of Process, "iecr" has crept in the class description.

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.

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.

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.

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.

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.

-Alan