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

[Mandy Chung]

On Thu, 27 May 2021 14:31:59 GMT, Aleksei Voitylov <avoitylov at openjdk.org> wrote:

>> Please review this PR which fixes the deadlock in ClassLoader between the two lock objects - a lock object associated with the class being loaded, and the ClassLoader.loadedLibraryNames hash map, locked during the native library load operation.
>> 
>> Problem being fixed:
>> 
>> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile and the hash map. That deadlock exists even when the ZipFile/JarFile lock is removed because there's another lock object in the class loader, associated with the name of the class being loaded. Such objects are stored in ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads exactly the same class, whose signature is being verified in another thread.
>> 
>> Proposed fix:
>> 
>> The proposed patch suggests to get rid of locking loadedLibraryNames hash map and synchronize on each entry name, as it's done with class names in see ClassLoader.getClassLoadingLock(name) method.
>> 
>> The patch introduces nativeLibraryLockMap which holds the lock objects for each library name, and the getNativeLibraryLock() private method is used to lazily initialize the corresponding lock object. nativeLibraryContext was changed to ThreadLocal, so that in any concurrent thread it would have a NativeLibrary object on top of the stack, that's being currently loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names of all native libraries loaded - in line with class loading code, it is not explicitly cleared.
>> 
>> Testing:  jtreg and jck testing with no regressions. A new regression test was developed.
>
> Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   address review comments

The change looks good in general with a minor suggestion to rename `NativeLibraryContext::get` to `NativeLibraryContext::current` which would be clearer as it returns the current native library context (of the current thread).

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.

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.

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.

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

test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java line 28:

> 26:  * @test
> 27:  * @bug 8266310
> 28:  * @summary deadlock while loading the JNI code

please update `@summary` with a description of the test (rather than the synposis of the bug).

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`.

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.

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.

test/jdk/java/lang/ClassLoader/loadLibraryDeadlock/TestLoadLibraryDeadlock.java line 165:

> 163: 
> 164:         runCommandInTestClassPath("rm", "-f", "*.jar")
> 165:                 .shouldHaveExitValue(0);

You can use `jdk.test.lib.util.FileUtils` to delete a directory or a given path.

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).

test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 83:

> 81:                 this.object = fromClass.newInstance();
> 82:                 this.method = fromClass.getDeclaredMethod("loadLibrary");
> 83:             } catch (Exception error) {

Nit: `s/Exception/ReflectiveOperationException/` as ROE is the specific checked exception you want to catch here.

test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 84:

> 82:                 this.method = fromClass.getDeclaredMethod("loadLibrary");
> 83:             } catch (Exception error) {
> 84:                 throw new Error(error);

Error is fine.  Most tests throw `RuntimeException` that can be another choice.

test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 92:

> 90:             try {
> 91:                 method.invoke(object);
> 92:             } catch (Exception error) {

Same here to catch the `ReflectiveOperationException`.

test/jdk/java/lang/ClassLoader/loadLibraryUnload/LoadLibraryUnload.java line 148:

> 146:         threads = null;
> 147:         exceptions.clear();
> 148:         waitForUnload(wClass);

`jdk.test.lib.util.ForceGC` can be used here.  You can reference test/jdk/java/lang/ClassLoader/nativeLIbrary/NativeLibraryTest.java.

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)

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

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