RFR 9: 8077350 Process API Updates Implementation Review

[Roger Riggs]

Hi Anthony,

Thanks for the review and comments.

On 4/11/2015 5:00 AM, Anthony Vanelverdinghe wrote:
> Hi Roger
>
> In my opinion, the method "supportsDestroyForcibly" is unintuitive, 
> for the following 2 reasons:
>
> - it's named "supportsXxx", where Xxx is the name of a method in the 
> same class. So as a user of this API, I would intuitively assume that 
> "supportsDestroyForcibly" is related to "destroyForcibly", and means 
> whether or not "destroyForcibly" actually forcibly terminates the 
> process.
>
> - "supports" has a positive connotation. However, if 
> "supportsDestroyForcibly" returns true, this means that the 
> implementation of "destroy" is forcible. And "destroyForcibly" either 
> invokes "destroy" (default implementation) or is overridden with a 
> compliant implementation. So in other words: if 
> "supportsDestroyForcibly" returns true, it's impossible to allow the 
> process to shut down cleanly. This, in my opinion, is a bad thing, so 
> the method indicating this behavior should not start with "supports".
The naming of the method has been problematic; as is the ambiguous 
semantics of Process.destroy
which are historical.
>
> Therefore I would like to propose to replace "supportsDestroyForcibly" 
> with "supportsDestroy", which returns the negation of what's currently 
> returned by "supportsDestroyForcibly".
I'm not sure this is clearer.  Both destroy() and destroyForcibly() 
always support the destruction
of the process and without reading more closely, supportDestroy() would 
always be true.

Perhaps supportNormalTermination() would reflect the more general 
understanding of the behavior
and the spec could provide the details in relation to the destroy method.

>
> Another question I have is: what happens if "destroy" or 
> "destroyForcibly" are called on processes other than the current 
> process & its children, i.e. a process that is in "allProcesses" but 
> isn't "current" &  isn't in "allChildren" (e.g. the parent process of 
> the current process)?
The current spec does not guarantee that the process will be destroyed;
processes can protect themselves against the signals (SIGTERM or SIGKILL)
just as it does not make any assertion about the timing of the process 
state change.

The behavior is to attempt to kill the process but it may not work for a 
variety of
reasons, not all of which can be detected or reported to the caller.
For ProcessHandle.destroy, if the OS does not allow the signal delivered,
the destory/destroyForcibly method could return a boolean.
Alternatively, Martin's request to throw arbitrary signals would be a 
new method
with more os specific behavior and exceptions.

The decoupling of ProcessHandle and Process opens the possibility
to have different semantics than for Process.destroy/destroyForcibly.

Roger
>
> Kind regards,
> Anthony
>
>
> On 9/04/2015 22:00, Roger Riggs wrote:
>> Please review the API and implementation of the Process API Updates
>> described inJEP 102 
>> <https://bugs.openjdk.java.net/browse/JDK-8046092>. Please review and 
>> comment by April 23rd.
>>
>> The recommendation to make ProcessHandle an interface is included
>> allowing the new functions to be extended by Process subclasses.
>> The implementation covers all functions on Unix, Windows, Solaris, 
>> and Mac OS X.
>>
>> The API doc: http://cr.openjdk.java.net/~rriggs/ph-apidraft/
>>
>> The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph
>>
>> Issue: JDK-8077350 <https://bugs.openjdk.java.net/browse/JDK-8077350> 
>> Process API Updates Implementation
>>
>> The code is in the jdk9 sandbox on branch JDK-8046092-branch.
>>
>> Please review and comment, Roger
>>
>>
>