On Fri, 21 May 2021 15:39:33 GMT, Aleksei Voitylov <avoity...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line >> 262: >> >>> 260: } finally { >>> 261: releaseNativeLibraryLock(name); >>> 262: } >> >> The new locking scheme looks incorrect to me. It now seems possible for 2 >> different class loaders in 2 different threads to load the same library >> (which should not be possible). >> >> Thread 1: >> - acquires name lock >> - checks library name is already in `loadedLibraryNames` (it's not) >> - release name lock >> - start loading library >> >> Then thread 2 comes in and does the same >> >> Then again thread 1 finishes loading and: >> - acquires name lock >> - puts library name in `loadedLibraryNames` >> - puts library name in it's own `libraries` >> - release lock >> >> Then thread 2 comes in and does the same again (although adding the name to >> `loadedLibraryNames` will silently return `false`). >> >> It seems that this scenario is possible, and the library will be loaded by 2 >> different class loaders, each with their own `lib` object. (If there's a >> race, there has to be a loser as well, which would need to discard their >> work when finding out they lost) >> >> You might be able to stress this by checking if >> `loadedLibraryNames.add(name);` returns `true`, and throwing an exception if >> not. >> >> I think a possible solution would be to claim the library name up front for >> a particular loader. Then 2 threads can still race to load the same library >> for the same class loader, but 2 threads with 2 different class loaders >> racing to load the library should not be possible. >> >> Actually, you might be able to prevent a race on JNI_OnLoad altogether by >> claiming the library name for a particular thread upfront as well (e.g. >> using a `ConcurrentHashMap<String, Thread>`). > > Thank you, the previous version of the PR suffered from other problems as > well. > > The PR is now reverted to a scheme with a lock held on library name, thus > such a situation is no longer possible. Ok, thanks. FWIW, I think trying to lock on a per-name basis instead of globally is a good idea, and should improve the current situation. It is also safe to do as far as I can see. ------------- PR: https://git.openjdk.java.net/jdk/pull/3976