ome / bioformats

Bio-Formats is a Java library for reading and writing data in life sciences image file formats. It is developed by the Open Microscopy Environment. Bio-Formats is released under the GNU General Public License (GPL); commercial licenses are available from Glencoe Software.
https://www.openmicroscopy.org/bio-formats
GNU General Public License v2.0
370 stars 241 forks source link

Bump file-leak-detector to version 1.18 for JDK 21 support #4158

Closed sbesson closed 4 months ago

sbesson commented 4 months ago

With the current infrastructure, the file-leak-detector fails with

test-automated:
   [testng] Could not load field socket from SocketImpl: java.lang.NoSuchFieldException: socket
   [testng] Could not load field serverSocket from SocketImpl: java.lang.NoSuchFieldException: serverSocket
   [testng] File leak detector installed
   [testng] FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed
   [testng] Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
   [testng] V  [libjvm.so+0x974f64]  jni_FatalError+0x74
   [testng] Exception in thread "main" java.lang.reflect.InvocationTargetException
   [testng] V  [libjvm.so+0xaddf24]  JvmtiExport::post_vm_initialized()+0x394
   [testng]     at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:118)
   [testng] V  [libjvm.so+0xea50c4]  Threads::create_vm(JavaVMInitArgs*, bool*)+0x8a4
   [testng]     at java.base/java.lang.reflect.Method.invoke(Method.java:580)
   [testng] V  [libjvm.so+0x97651e]  JNI_CreateJavaVM+0x4e
   [testng]     at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:560)
   [testng] C  [libjli.so+0x42cb]  JavaMain+0x8b
   [testng]     at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:572)
   [testng] C  [libjli.so+0x7e49]  ThreadJavaMain+0x9
   [testng] Caused by: java.lang.ClassNotFoundException: java.net.PlainSocketImpl
   [testng]     at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
   [testng]     at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
   [testng]     at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526)
   [testng]     at java.base/java.lang.Class.forName0(Native Method)
   [testng]     at java.base/java.lang.Class.forName(Class.java:421)
   [testng]     at java.base/java.lang.Class.forName(Class.java:412)
   [testng]     at org.kohsuke.file_leak_detector.AgentMain.premain(AgentMain.java:135)
   [testng]     at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
   [testng]     ... 3 more
   [testng] *** java.lang.instrument ASSERTION FAILED ***: "!errorOutstanding" with message Outstanding error when calling method in invokeJavaAgentMainMethod at open/src/java.instrument/share/native/libinstrument/JPLISAgent.c line: 627
   [testng] *** java.lang.instrument ASSERTION FAILED ***: "success" with message invokeJavaAgentMainMethod failed at open/src/java.instrument/share/native/libinstrument/JPLISAgent.c line: 466
   [testng] *** java.lang.instrument ASSERTION FAILED ***: "result" with message agent load/premain call failed at open/src/java.instrument/share/native/libinstrument/JPLISAgent.c line: 429

This tries to upgrade the version of the file leak detector library to 1.18 which has allegedly JDK 21 support

melissalinkert commented 4 months ago

As discussed separately with @sbesson, 1.16 and later quietly removed Java 8 support. https://github.com/jenkinsci/lib-file-leak-detector/pull/98 and https://github.com/jenkinsci/lib-file-leak-detector/pull/99 in particular appear to be the relevant changes.

I can't imagine we can drop Java 8 support quite yet, so probably we can't upgrade file-leak-detector for now.

Since file-leak-detector 1.15 with Java 21 results in a runtime error instead of a compile error, I tried just turning off file-leak-detector based on the detected Java version. Something like this seems to work with file-leak-detector 1.15 and Java 21 locally:

diff --git a/components/test-suite/build.xml b/components/test-suite/build.xml
index 0633b4cefc..abd56ba1c7 100644
--- a/components/test-suite/build.xml
+++ b/components/test-suite/build.xml
@@ -6,7 +6,7 @@ Download Apache Ant from http://ant.apache.org/.
 Type "ant -p" for a list of targets.
 -->

-<project name="tests" default="jar" basedir=".">
+<project name="tests" default="jar" basedir="." xmlns:if="ant:if">
   <description>Build file for Bio-Formats testing framework project</description>
   <property name="root.dir" location="../.."/>
   <import file="${root.dir}/ant/java.xml"/>
@@ -236,6 +236,12 @@ Type "ant -p" for a list of targets.
   <target name="test-automated" depends="compile-tests"
     description="run automated tests in group 'automated'">

+    <condition property="compatibleJavaVersion">
+      <not>
+        <equals arg1="${ant.java.version}" arg2="21"/>
+      </not>
+    </condition>
+
     <testng groups="automated" testname="Automated tests"
       listeners="loci.tests.testng.OrderingListener"
       suitename="Bio-Formats software test suite"
@@ -265,7 +271,7 @@ Type "ant -p" for a list of targets.
       <jvmarg value="-Duser.language=${user.language}"/>
       <jvmarg value="-Duser.country=${user.country}"/>
       <jvmarg value="-Dlogback.configurationFile=${logback.configurationFile}"/>
-      <jvmarg value="-javaagent:${dep.org.kohsuke:file-leak-detector:jar:jar-with-dependencies}=strong"/>
+      <jvmarg if:set="compatibleJavaVersion" value="-javaagent:${dep.org.kohsuke:file-leak-detector:jar:jar-with-dependencies}=strong"/>
     </testng>
     <fail if="failedTest"/>
   </target>

That obviously means we're not checking for leaking file handles in Java 21, but maybe that's OK given that we test with 3 other versions?

I wouldn't be opposed to trying to replace file-leak-detector with something else, but I don't know of a reasonable alternative to consider.

sbesson commented 4 months ago

Thanks @melissalinkert for the detailed investigation and agreed on all the points above. Do you want to close this PR and open a separate one with your conditional logic skipping the file leak detection on JDK 21 for evaluation of the nightly CI builds?

melissalinkert commented 4 months ago

Sounds good - see https://github.com/ome/bioformats/pull/4159.