On Thu, 27 May 2021 14:31:59 GMT, Aleksei Voitylov <avoity...@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
