RFR: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll [v2]

[Chris Hegarty]

On Mon, 15 Mar 2021 12:53:16 GMT, Jonathan Dowland <jdowland at openjdk.org> wrote:

>> This is an adaptation of a patch originally written by Shafi Ahmad in
>> a comment on the JBS page but never submitted or merged.
>> 
>> With -Xcheck:jni, the method java.net.NetworkInterface.getAll very
>> quickly breaches the default JNI local refs threshold (32). Exactly when
>> this happens depends upon the number of network interfaces (in state "UP")
>> visible to Java. On Linux, with current Trunk, 2 network interfaces is
>> sufficient to breach:
>> 
>>     $ ./addif.sh 0
>>     1 interfaces
>>     $ sudo ip netns exec jbase $JAVA_HOME/bin/java -Xcheck:jni NITest
>>      (nothing)
>>     $ ./addif.sh 1
>>     2 interfaces
>>     $ sudo ip netns exec jbase $JAVA_HOME/bin/java -Xcheck:jni NITest
>>     WARNING: JNI local refs: 33, exceeds capacity: 32
>>             at java.net.NetworkInterface.getAll(java.base/Native Method)
>>             at java.net.NetworkInterface.getNetworkInterfaces(java.base/NetworkInterface.java:351)
>>             at NITest.main(NITest.java:3)
>> 
>> This patch improves the situation:
>> 
>>     $ ./addif.sh 3
>>     4 interfaces
>>     $ sudo ip netns exec jbase $JAVA_HOME/bin/java -Xcheck:jni NITest
>>      (nothing)
>>     $ ./addif.sh 4
>>     5 interfaces
>>     $ sudo ip netns exec jbase $JAVA_HOME/bin/java -Xcheck:jni NITest
>>     WARNING: JNI local refs: 33, exceeds capacity: 32
>>             at java.net.NetworkInterface.getAll(java.base/Native Method)
>>             at java.net.NetworkInterface.getNetworkInterfaces(java.base/NetworkInterface.java:351)
>>             at NITest.main(NITest.java:3)
>> 
>> 
>> Once the JNI local refs threshold is breached, the threshold is raised.
>> With the patch, it takes 10 network interfaces to breach the new
>> threshold 
>> 
>>     $ ./addif.sh 9
>>     10 interfaces
>>     $ sudo ip netns exec jbase $JAVA_HOME/bin/java -Xcheck:jni NITest
>>     WARNING: JNI local refs: 33, exceeds capacity: 32
>>             at java.net.NetworkInterface.getAll(java.base/Native Method)
>>             at java.net.NetworkInterface.getNetworkInterfaces(java.base/NetworkInterface.java:351)
>>             at NITest.main(NITest.java:3)
>>     WARNING: JNI local refs: 66, exceeds capacity: 65
>>             at java.net.NetworkInterface.getAll(java.base/Native Method)
>>             at java.net.NetworkInterface.getNetworkInterfaces(java.base/NetworkInterface.java:351)
>>             at NITest.main(NITest.java:3)
>> 
>> Without the patch it takes 5.
>> 
>> Helper scripts for testing on Linux. `setupnet.sh`:
>> 
>>     #!/bin/bash
>>     set -euo pipefail
>> 
>>     namespace=${namespace:-jbase}
>>     sudo ip netns add ${namespace}
>> 
>> And `addif.sh`:
>> 
>>     #!/bin/bash
>>     set -euo pipefail
>> 
>>     namespace=${namespace:-jbase}
>>     num="$1"
>> 
>>     sudo ip link add name vethhost${num} type veth peer name veth${namespace}${num}
>>     sudo ip link set veth${namespace}${num} netns ${namespace}
>>     sudo ip addr add 192.168.2.${num}/24 dev vethhost${num}
>>     sudo ip netns exec ${namespace} ip addr add 192.168.2.${num}/24 dev veth${namespace}${num}
>>     sudo ip link set vethhost${num} up
>>     sudo ip netns exec ${namespace} ip link set veth${namespace}${num} up
>> 
>>     count="$(sudo ip netns exec ${namespace} ip link show |grep UP | wc -l)"
>>     echo "${count} interfaces"
>
> Jonathan Dowland has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll

src/java.base/unix/native/libnet/NetworkInterface.c line 773:

> 771:                         return NULL;
> 772:                     }
> 773:                     (*env)->DeleteLocalRef(env, ia2Obj);

The pattern is 1) SetObjectXXX, followed by 2) DeleteLocalRef (once there is another, non-local, reference). It might be worth relocating this line of code to directly after the SetObjectField.

src/java.base/unix/native/libnet/NetworkInterface.c line 813:

> 811:         addrP = addrP->next;
> 812:         (*env)->DeleteLocalRef(env, iaObj);
> 813:         (*env)->DeleteLocalRef(env, ibObj);

At the cost of duplicating the delete on ibObj, is it worth locating the DeleteLocalRef to directly after the SetObjectArrayElement (so as to make it a little easier to reason about whether the code has followed the set/delete pattern correctly) ?

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

PR: https://git.openjdk.java.net/jdk/pull/2963