On Fri, 21 May 2021 15:49:09 GMT, Aleksei Voitylov <[email protected]>
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
Hi Aleksei,
Thanks for bearing with me while I got my head round the synchronization
protocol - piggy-backing on the CHM logic is clever! That locking part all
makes sense to me now and I think it is a good solution to reduce the scope of
the deadlock potential.
I'm less clear on the changes to the NativeLibraryContext as I'm not certain
what it means today, so making it a per-thread mapping is not something I can
really comment on. I'll leave that aspect to core-libs folk. So my approval
should not be taken as an approval to push yet.
Thanks,
David
src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 497:
> 495:
> 496: private static void acquireNativeLibraryLock(String libraryName) {
> 497: nativeLibraryLockMap.compute(libraryName, (name, lock) -> {
This would be clearer if lock were named currentLock as for releaseNLLock
-------------
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3976
