RFR: 8357910: LoaderConstraintsTest.java fails when run with TEST_THREAD_FACTORY=Virtual [v2]

[Coleen Phillimore]

On Mon, 2 Jun 2025 22:44:58 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> test/hotspot/jtreg/runtime/logging/LoaderConstraintsTest.java line 56:
>> 
>>> 54:             Class<?> c = cl.loadClass(className);
>>> 55:             cl = null; c = null;
>>> 56:             ClassUnloadCommon.triggerUnloading();
>> 
>> So the unloading is not part of this test? Are we just using the ClassUnloadCommon utility as a convenient way to get an additional classloader involved?
>
> Yes, the unloading part is not needed. Before this patch, the only line needed was `ClassLoader cl = ClassUnloadCommon.newClassLoader();`. That would already trigger the constraint we checked for in the parent. But it doesn't work for virtual threads, so I changed it to check for a constraint on the created `ClassUnloadCommonClassLoader` instead to avoid dependencies on previously executed code like with the `app` classloader.

I think we added the class unloading so that we can see the -Xlog output for that.  But it's not reliable enough so we might have removed it.  This is fine to remove it.

>> test/hotspot/jtreg/runtime/logging/LoaderConstraintsTest.java line 66:
>> 
>>> 64:         Collections.addAll(argsList, args);
>>> 65:         Collections.addAll(argsList, "-Xmn8m");
>>> 66:         Collections.addAll(argsList, "-Xbootclasspath/a:.");
>> 
>> Without use of `WhiteBox`, you don't need this. And this is probably what caused the class to be loaded by the boot-loader instead of the custom loader as expected. But the use of WB is needed by `ClassUnloadCommon` and these instructions are present to ensure it works - as added by JDK-8289184.
>
> Added back the use of `WhiteBox`. As to `test.Empty`, it was always loaded by the `app` classloader regardless of `-Xbootclasspath/a:.`, i.e that’s only needed for loading `WhiteBox`. The issue was the missing `test.class.path` property. Class `test.Empty` resides in directory `jtreg_test_hotspot_jtreg_runtime_logging_LoaderConstraintsTest_java/classes/0/runtime/logging/classes/test/` but the process’s current working directory is `jtreg_test_hotspot_jtreg_runtime_logging_LoaderConstraintsTest_java/scratch/0`, so setting the custom classloader classpath as `.` meant it wasn’t able to find it. So we ended up delegating to the parent classloader (`app`) which already had the correct directories in the classpath.

Good work figuring that out.  yes, ClassUnloadCommon needs this because it uses WhiteBox for class unloading.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25496#discussion_r2123643517
PR Review Comment: https://git.openjdk.org/jdk/pull/25496#discussion_r2123640939