RFR 9: 8078099 : (process) ProcessHandle should uniquely identify processes

[Roger Riggs]

Hi Peter,

Thanks for reviewing the new implementation.

The idea to pre-allocate the buffer based on the size of the output 
arrays passed for output
will not work as desired.
The same native code entry point is used whether it is a query for all 
processes or
just the immediate children.  In either case, the buffer for sysctl 
needs to be
large enough to accommodate all of the OS threads; its not a function of the
output array size.

With respect to the buffer allocation on OS X, I might see the point of 
a retry
when sysctl returns ENOMEM;


I don't think its particularly worthwhile to add the native code to 
retry in the case
of ENOMEM. The OSX doc for sysctrl does indicate it tries to round up
to avoid a subsequent failure.
The ProcessHandle API already cautions that the list of processes is dynamic
and that processes may started or terminate concurrently with the call  
to children/allChildren.
So if the sysctl returns ENOMEM indicating not all the processes fit; 
the ones
that do fit within in the allocated buffer are valid at that point in time.
If there are extras that do not fit in the buffer they are likely to be 
'newer'
processes and can be considered to have been started 'after' the 
children/allChildren invocation.
So I propose to change the error checking to consider ENOMEM as a 
non-exceptional
return and return the processes available.

Thanks, Roger



On 7/2/2015 3:32 AM, Peter Levart wrote:
> Hi Roger,
>
> I looked at the code briefly and have the following comments:
>
> For ProcessHandleImpl_macosx.c, in method getProcessPids0:
>
>  224     // Get buffer size needed to read all processes
>  225     int mib[4] = {CTL_KERN, KERN_PROC, KERN_PROC_ALL, 0};
>  226     if (sysctl(mib, 4, NULL, &bufSize, NULL, 0) < 0) {
>  227         JNU_ThrowByNameWithLastError(env,
>  228             "java/lang/RuntimeException", "sysctl failed");
>  229         return -1;
>  230     }
>  231
>  232     // Allocate buffer big enough for all processes
>  233     void *buffer = malloc(bufSize);
>  234     if (buffer == NULL) {
>  235         JNU_ThrowOutOfMemoryError(env, "malloc failed");
>  236         return -1;
>  237     }
>  238
>  239     // Read process info for all processes
>  240     if (sysctl(mib, 4, buffer, &bufSize, NULL, 0) < 0) {
>  241         JNU_ThrowByNameWithLastError(env,
>  242             "java/lang/RuntimeException", "sysctl failed");
>  243         free(buffer);
>  244         return -1;
>  245     }
>
> ... the 1st call to sysctl is used to measure the size of the needed 
> buffer and the 2nd call fills-in the buffer. The documentation for 
> sysctl says:
>
> "     The information is copied into the buffer specified by oldp. The 
> size of the buffer is given by the
>      location specified by oldlenp before the call, and that location 
> gives the amount of data copied after
>      a successful call and after a call that returns with the error 
> code ENOMEM.  If the amount of data
>      available is greater than the size of the buffer supplied, the 
> call supplies as much data as fits in
>      the buffer provided and returns with the error code ENOMEM. If 
> the old value is not desired, oldp and
>      oldlenp should be set to NULL.
>
>      The size of the available data can be determined by calling 
> sysctl() with the NULL argument for oldp.
>      The size of the available data will be returned in the location 
> pointed to by oldlenp.  For some opera-tions, operations,
>      tions, the amount of space may change often.  For these 
> operations, the system attempts to round up so
>      that the returned size is large enough for a call to return the 
> data shortly thereafter."
>
> So while not very probable, it can happen that you get ENOMEM from 2nd 
> call because of underestimated buffer size. Would it be better to 
> retry (re)allocation of buffer and 2nd call in a loop with new 
> estimation returned from previous call while the error is ENOMEM?
>
> Another suggestion: What would it be if the buffer size estimation was 
> not computed by a call to sysctl with NULL buffer, but was taken from 
> the passed-in resulting array size(s). In case the user passes-in 
> arrays of sufficient size, you can avoid double invocation of sysctl. 
> Also, if ENOMEM happens, you can just return the result obtained and 
> the new estimation - no looping in native code. The UNIX, Solaris and 
> Windows variants look good.
>
> Regards, Peter
>
> On 06/22/2015 05:10 PM, Roger Riggs wrote:
>> Please review changes to ProcessHandle implementation to uniquely 
>> identify
>> processes based on the start time provided by the OS. It addresses 
>> the issue
>> of PID reuse.
>>
>> This is the implementation of the ProcessHandle.equals() spec change in
>>     8129344 : (process) ProcessHandle instances should define equals 
>> and be value-based
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-starttime-8078099/
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8078099
>>
>> Thanks, Roger
>>
>