RFR 9: 8077350 Process API Updates Implementation Review

[Roger Riggs]

Hi Martin,

Thanks for the review.

On 4/11/2015 1:37 AM, Martin Buchholz wrote:
> Thanks for the huge effort.  I did a superficial review and it seems 
> pretty good.  Of course, changing the Process good is high risk and 
> some things will probably need fixup later.
>
> On Unix, you seem to be identifying ProcessHandles by their pid.  What 
> are you going to do about the problem of misidentifying Unix processes 
> whose pid got recycled?  If you spin up a thread to call waitpid as 
> soon as you create the ProcessHandle, the window is very short (and 
> OSes avoid recycling pids immediately) so that's pretty safe, but it 
> wastes a thread for each ProcessHandle!  I don't have a good answer.  
> Maybe record the start time of the process, where available?
Getting the basic API and implementation in has taken too long, so I'm 
looking to get it general agreement
and pushed and then I'd be happy to go back and include the start time 
in ProcessHandle.
In the case of children() which already has to read /proc/pid/stat for 
the parent,
the start time is inexpensively available also.  The spec for equals(), 
and compareTo is straightforward
without the start time.  With the start time, either starttime has be 
exposed in the API/spec or
be left 'implementation' dependent which is a poor spec.
Only in the case of allProcesses() would the overhead increase 
significantly, it currently
does not need to open /proc/pid for every process.

>
> +        } else if (siginfo.si_code == CLD_KILLED || siginfo.si_code == CLD_DUMPED) {
> Using siginfo probably won't work under load because multiple signals 
> arriving at the same time are coalesced into one, at least on Linux.
> Unix signals are sadly totally broken.
Siginfo is only used with waitid as an output argument

For the case of reaping the status, it uses waitpid (as Process did 
before),
that seemed the least risky.
For the case of waiting for a process without reaping, that option is 
only available
using waitid.  I tried using waitid for both cases but on Mac, waitid 
didn't seem to
be working correctly in all cases.

>
> --
>
> It seems you don't support sending arbitary signals, which is a little 
> sad.
I had not investigated any potential negative and unknowable effects of 
being able to send arbitrary signals.
The implementation is easy to generalize, though it does not map to 
Windows well/at all.
Would you propose it as an 'int' argument with no limitation? or what 
limits?
I think it would be an additional method, the current 
destory/destroyForcibly.

Thanks, Roger

>
>
> On Thu, Apr 9, 2015 at 1:00 PM, Roger Riggs <Roger.Riggs at oracle.com 
> <mailto:Roger.Riggs at oracle.com>> 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/
>     <http://cr.openjdk.java.net/%7Erriggs/ph-apidraft/>
>
>     The webrev: http://cr.openjdk.java.net/~rriggs/webrev-ph
>     <http://cr.openjdk.java.net/%7Erriggs/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
>
>