On Fri, 21 May 2021 15:49:09 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:
>
> fix whitespace
Changes requested by plevart (Reviewer).
src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 481:
> 479: throw new Error("Maximum lock count exceeded");
> 480: }
> 481:
Hi Aleksei,
I know in practice this counter will never overflow, but just to be pedantic,
it would be better that you either decrement back the counter before throwing
Error or you check for overflow without incrementing it. Otherwise the lock is
left in a corrupted state since you use it like this:
acquireNativeLibraryLock(name);
try {
...
} finally {
releaseNativeLibraryLock(name);
}
...you see, if acquire fails, it is not paired with release, which is correct
since the ReentrantLock has not been acquired, but the counter *HAS* been
incremented...
-------------
PR: https://git.openjdk.java.net/jdk/pull/3976
