On Fri, 15 Nov 2024 21:31:54 GMT, Brent Christian <bchri...@openjdk.org> wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> prev is not needed > > src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java line 310: > >> 308: } >> 309: assert phc.node.arr[phc.index] == phc; >> 310: assert head.size > 0; > > I now realize that these assertions in `remove()` won't really "work" because > _"exceptions thrown by the cleaning action are ignored"_. :( > When the cleaner thread removes the phc (`ref.clean()` in `run()` on L142 -> > `list.remove()` on PhantomCleanable L94 ), any `AssertionError`s will get > lost in `run()`'s `catch` statement, L144. > > (There is a related RFE [JDK-8305979 : UncaughtExceptionHandler for > Cleaner](https://bugs.openjdk.org/browse/JDK-8305979)) Ah, true. Yes, this is inconvenient, but I think these asserts are opportunistic to catch internal errors, so they are of interest of JDK testing and catching JDK bugs, not the issues in the user code in Cleaners. I see the asserts would fire from the direct invocation of `clean()`, as deliberately breaking the asserts in `remove()` gets me this failure immediately: Running test 'jtreg:test/jdk/java/lang/ref' Directory "/Users/shipilev/Work/shipilev-jdk/build/macosx-aarch64-server-release/test-results/jtreg_test_jdk_java_lang_ref" not found: creating Error: A JNI error has occurred, please check your installation and try again Exception in thread "main" java.lang.AssertionError at java.base/jdk.internal.ref.CleanerImpl$CleanableList.remove(CleanerImpl.java:310) at java.base/jdk.internal.ref.PhantomCleanable.clean(PhantomCleanable.java:94) at java.base/java.util.zip.ZipFile$ZipFileInflaterInputStream.close(ZipFile.java:416) at java.base/java.util.jar.JarFile.getBytes(JarFile.java:803) at java.base/java.util.jar.JarFile.checkForSpecialAttributes(JarFile.java:1006) at java.base/java.util.jar.JarFile.hasClassPathAttribute(JarFile.java:954) at java.base/java.util.jar.JavaUtilJarAccessImpl.jarFileHasClassPathAttribute(JavaUtilJarAccessImpl.java:34) at java.base/jdk.internal.loader.URLClassPath$JarLoader.getClassPath(URLClassPath.java:922) at java.base/jdk.internal.loader.URLClassPath.getLoader(URLClassPath.java:447) at java.base/jdk.internal.loader.URLClassPath.getResource(URLClassPath.java:314) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:757) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:497) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:577) at java.base/java.lang.Class.forName(Class.java:556) at java.base/sun.launcher.LauncherHelper.loadMainClass(LauncherHelper.java:854) at java.base/sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:739) So I don't regard this as blocking. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22043#discussion_r1846180271
