RFR: 8266310: deadlock while loading the JNI code [v6]

[Aleksei Voitylov]

On Mon, 31 May 2021 20:56:14 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   address review comments
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 207:
> 
>> 205:              *
>> 206:              * We use a static stack to hold the list of libraries we are
>> 207:              * loading, so that each thread maintains its own stack.
> 
> line 206: no longer a static stack.  line 206-207 can be replaced with:
> 
> 
>              * Each thread maintains its own stack to hold the list of libraries it is loading.

Done.

> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 213:
> 
>> 211:              * a different class loader, we raise UnsatisfiedLinkError.
>> 212:              */
>> 213:             for (NativeLibraryImpl lib : NativeLibraryContext.get()) {
> 
> I suggest to rename the `get` method of `NativeLibraryContext` to `current` that returns the current thread's context.

Done.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/LoadLibraryDeadlock.java line 49:
> 
>> 47:                          InstantiationException |
>> 48:                          IllegalAccessException ignore) {
>> 49:                     System.out.println("Class Class1 not found.");
> 
> In general a test should let the exception to propagate.   In this case, the threads (both t1 and t2) can warp the exception and throw `RuntimeException` as it's an error.

Fixed.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/LoadLibraryDeadlock.java line 60:
> 
>> 58:                     System.out.println("Signed jar loaded.");
>> 59:                 } catch (ClassNotFoundException ignore) {
>> 60:                     System.out.println("Class Class2 not found.");
> 
> Same as above

Fixed.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java line 69:
> 
>> 67: 
>> 68:     private static OutputAnalyzer genKey() throws Throwable {
>> 69:         runCommandInTestClassPath("rm", "-f", KEYSTORE);
> 
> Can you use `jdk.test.lib.util.FileUtils` instead of exec `rm -f`.

Done.

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java line 84:
> 
>> 82: 
>> 83:     private static OutputAnalyzer createJar(String outputJar, String... classes) throws Throwable {
>> 84:         String jar = JDKToolFinder.getJDKTool("jar");
> 
> You can consider using `jdk.test.lib.util.JarUtils` to reduce the test execution time so that it can create the jar without exec'ing another process.

I tried, but this doesn't work that well unfortunately with JarUtils.createJar() as it only produces a valid jar-file capable of triggering the issue when the compiled class files (jtreg @build output) are in the same directory as the current directory of the process calling JarUtils.createJar(). Jtreg @build outputs the compiled classes to a separate directory. Creation of a new jar process allows to change the current directory so that the relative paths to the class files are such that it would form the valid output jar-file. Alternatively we could copy the class files to the current directory prior to creating the jar-file, but that would introduce an extra step. Would you mind me keeping the createJar() in the test?

> test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java line 141:
> 
>> 139:         try {
>> 140:             return future.get(1000, TimeUnit.MILLISECONDS);
>> 141:         } catch (Exception ignoreAll) {
> 
> if this is an unexpected error case, it should throw an exception.

If this threw an exception, it wouldn't be possible to collect all available data from the process which is not yet completed. Throwing an exception would defeat the purpose of readAvailable() method.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 31:
> 
>> 29:  * @test
>> 30:  * @bug 8266310
>> 31:  * @summary deadlock while loading the JNI code
> 
> please update `@summary` with a description of the test (rather than the synposis of the bug).

Updated.

> test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnloadTest.java line 32:
> 
>> 30:  * @test
>> 31:  * @bug 8266310
>> 32:  * @summary deadlock while loading the JNI code
> 
> Please update `@summary` with a description of the test (rather than the synposis of the bug)

Updated.

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

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