RFR: JDK-8315026: java/lang/ProcessHandle/TreeTest.java fails intermittent on AIX in TreeTest.test5 [v4]

[Thomas Stuefe]

On Wed, 11 Oct 2023 10:57:24 GMT, Joachim Kern <jkern at openjdk.org> wrote:

>> We see rather often failures in java/lang/ProcessHandle/TreeTest.java on AIX in TreeTest.test5.
>> The reason is: Previously the implementation based on the /proc file system lead to double pids in the child list; at least intermittent. Using the API getprocs64() instead I was able to eliminate those double pids (and increase the performance by a factor of 4). Otherwise we would have to add an algorithm to filter out the doubles after creating the raw list.
>
> Joachim Kern has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cosmetic changes 2

src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 69:

> 67: 
> 68:         if (arraySize != parentArraySize) {
> 69:             JNU_ThrowIllegalArgumentException(env, "array sizes not equal");

proposal: "Parent pid array must have the same length as result pid array"

src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 89:

> 87: 
> 88:     do { // Block to break out of on Exception
> 89:         pids = (*env)->GetLongArrayElements(env, jarray, NULL);

Nit, I'd move these invocations of GLAE into the initialization block and remove the outer loop. I see what you do here, the outer block gives you a reliable jumping point to get cleanup in case of an error. But I would just use a goto cleanup label. That's clearer and allows you to handle initialization in one place.

Proposal for initialization:


if (jparentArray != null) {
  - check size is same as primary array, if wrong throw + goto cleanup
  - ppids = GLAE
} else {
  - if input pid is 0, throw an error too, since for "get all processes mode"
      we need the parent array. Throw + goto cleanup.
}
// same for times array

src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 112:

> 110:                 jlong startTime = 0L;
> 111: 
> 112:                 /* skip files that aren't numbers */

As @MBaesken pointed out, this comment is obsolete.

src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 114:

> 112:                 /* skip files that aren't numbers */
> 113:                 pid_t childpid = (pid_t) ProcessBuffer[i].pi_pid;
> 114:                 if ((int) childpid <= 0) {

Can this even happen? Negative pids are group pids, I don't think getprocs returns group pids.

src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 120:

> 118:                 // Get the parent pid, and start time
> 119:                 ppid = (pid_t) ProcessBuffer[i].pi_ppid;
> 120:                 startTime = ((jlong) ProcessBuffer[i].pi_start) *1000;

nit: space before 1000

src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 121:

> 119:                 ppid = (pid_t) ProcessBuffer[i].pi_ppid;
> 120:                 startTime = ((jlong) ProcessBuffer[i].pi_start) *1000;
> 121:                 if (ppid >= 0 && (pid == 0 || ppid == pid)) {

I think the first condition is always true. You can remove it.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356131767
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356134470
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356137028
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356137647
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356137949
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356141163