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

[Alan Bateman]

On 24/04/2015 16:49, Roger Riggs wrote:
> :
>>
>> 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.
Right, I'm not suggesting that, just pointing out @implSpec in the 
webrev doesn't specify how the default implementation works. It instead 
specifies what ProcessBuilder#start returns. Is the javadoc and webrev 
in sync, that might be part of the confusion on my part.

>
>> 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.
Okay, if you've settled on this.

>
>>
>> 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.
If you keep it then some commentary would be helpful as the reader might 
not understand the term.

The parent is optional so something to consider.



>>
>> 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.
Okay although not having "CpuTime" and "StartTime" in the method names 
means the names will be less clear.

-Alan