RFR 9: 8077350 Process API Updates Implementation Review

[Anthony Vanelverdinghe]

Thanks for your clarifications Roger. I'm very much in favor of your 
suggestion for naming the method "supportsNormalTermination".

Kind regards,
Anthony


On 11/04/2015 20:35, Roger Riggs wrote:
> 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
>>>
>>>
>>
>
>
>