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