Re: RFR: 8266310: deadlock while loading the JNI code [v2]
Peter, David, I updated the PR to focus on solving the problem I originally intended to solve that is observed in the wild - a deadlock when two different libraries are being loaded. The case when the same library JNI_OnLoad has FindClass with a circular dependency on loading the same library is artificial. I thought of two ways to resolve it with the following conclusions: - not holding a lock when JNI_OnLoad is executed can only be done in a such a way that would lead to a possibility to get JNI_OnLoad executed multiple times, and the JNI code can rely on the fact that it's called only once (though the JNI spec does not say so). Decoupling dlopen/LoadLibrary from JNI_OnLoad does not solve this problem as well. - throwing something like LibraryBeingLoadedError similar to ClassCircularityError for JNI calls trying to use a class which initializes the same library is also not a good option, as it is behavior change. I as a user would not know what to do with that (remember it can appear for a different thread that just happened to be initializing the same library as well). Such a thread should just sit and wait on the lock. Both such solutions would make innocent code suffer, so are a no-go. I'm open to ideas how to resolve it, if at all, but IMO it has a different priority compared to the original bug report. I thus focused on solving the original problem. Thanks, -Aleksei On 21/05/2021 21:10, Peter Levart wrote: > > On 21/05/2021 10:29, Peter Levart wrote: >> I still haven't found a scenario of a possible deadlock when Sergei's >> initial patch is combined with... > > > Sorry Aleksei, I renamed you to Sergei without intention. Please > excuse me. > > > Peter > >
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
On 21/05/2021 10:29, Peter Levart wrote: I still haven't found a scenario of a possible deadlock when Sergei's initial patch is combined with... Sorry Aleksei, I renamed you to Sergei without intention. Please excuse me. Peter
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
On 21/05/2021 01:11, David Holmes wrote: Hi Peter, On 21/05/2021 12:42 am, Peter Levart wrote: Hi Aleksei, Are you trying to solve this in principle or do you have a concrete problem at hand which triggers this deadlock? If it is the later, then some rearrangement of code might do the trick... For example, native libraries are typically loaded by a class initializer of some class that is guaranteed to be initialized before the 1st invocation of a native method from such library. But if such class can also be loaded and initialized by some other trigger, deadlock can occur. Best remedy for such situation is to move all native methods to a special class that serves just for interfacing with native code and also contains an initializer that loads the native library and nothing else. Such arrangement would ensure that the order of taking locks is always the same: classLoadingLock -> nativeLibraryLock ... There were specific examples for this problem, but Aleksei is trying to define a general solution - which unfortunately doesn't exist. The basic deadlock scenario is a special variant of the general class initialization deadlock: Thread 1: - loadLibrary - acquire loadLibrary global lock - call JNI_OnLoad - use class Foo (which needs to be loaded and initialized) - block acquiring lock for Foo Thread 2: - Initialize class Foo - Acquire lock for Foo - - call loadLibrary(x) // for any X - block acquiring loadLibrary global lock We can reduce the chance of deadlock by using a per-native-library lock instead of the global loadLibrary lock - which is what Aleksei's initial version did. But we cannot remove all deadlock possibility because we must ensure only one thread can be executing JNI_OnLoad for any given native library. Right, I was just trying to suggest that by exercising some discipline about how to arrange code that loads native libraries, deadlocks can be avoided if also Aleksei's initial version of the patch is used (using a per-native-library lock instead of the global loadLibrary lock). The discipline would be to load a particular native library only from of a unique class associated with that native library. If everybody followed this discipline (which fortunately is typical use of loadLibrary), then without Aleksei's patch, deadlock is still possible. Imagine two native libraries A and B, each loaded from of corresponding classes ClassA and ClassB. Library A also has JNI_OnLoad which uses ClassB. So we have: Thread 1: - initialize ClassA - acquire lock for ClassA - ClassA. - call loadLibrary(A) - acquire loadLibrary global lock - call JNI_OnLoad for A - use class ClassB (which needs to be loaded and initialized) - block acquiring lock for ClassB Thread 2: - initialize ClassB - acquire lock for ClassB - ClassB. - call loadLibrary(B) - block acquiring loadLibrary global lock With Aleksei's initial patch, such scenario would not result in a deadlock. And since such scenario can arise from typical use of loadLibrary, I think this patch is a good thing. It reduces number of scenarios where deadlock is a possible outcome. Here's another scenario where deadlock would still occur even with Sergei's initial patch. It is a modified variant of above scenario where JNI_OnLoad of library A uses ClassB and JNI_OnLoad of library B uses ClassA: Thread 1: - initialize ClassA - acquire lock for ClassA - ClassA. - call loadLibrary(A) - acquire loadLibrary(A) lock - call JNI_OnLoad for A - use class ClassB (which needs to be loaded and initialized) - block acquiring lock for ClassB Thread 2: - initialize ClassB - acquire lock for ClassB - ClassB. - call loadLibrary(B) - acquire loadLibrary(B) lock - call JNI_OnLoad for B - use class ClassA (which needs to be loaded and initialized) - block acquiring lock for ClassA But in this scenario, deadlock is provoked without blocking on any loadLibrary lock. This deadlock arises from circular dependencies among classes in their methods. It just happens that these dependencies are evaluated via a chain of: X. -> loadLibrary -> JNI_OnLoad -> use class Y calls. Such deadlock is possible without involvement of native libraries loading too, so I would not classify it as the problem that Sergei's patch is trying to solve. I still haven't found a scenario of a possible deadlock when Sergei's initial patch is combined with the above mentioned discipline in which a loadLibrary lock would be involved. Regards, Peter Cheers, David - Regards, Peter On 5/20/21 12:31 AM, David Holmes wrote: On 20/05/2021 2:29 am, Aleksei Voitylov wrote: On Wed, 19 May 2021 16:21:41 GMT, Aleksei Voitylov wrote: Please review this PR which fixes
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
Hi Peter, On 21/05/2021 12:42 am, Peter Levart wrote: Hi Aleksei, Are you trying to solve this in principle or do you have a concrete problem at hand which triggers this deadlock? If it is the later, then some rearrangement of code might do the trick... For example, native libraries are typically loaded by a class initializer of some class that is guaranteed to be initialized before the 1st invocation of a native method from such library. But if such class can also be loaded and initialized by some other trigger, deadlock can occur. Best remedy for such situation is to move all native methods to a special class that serves just for interfacing with native code and also contains an initializer that loads the native library and nothing else. Such arrangement would ensure that the order of taking locks is always the same: classLoadingLock -> nativeLibraryLock ... There were specific examples for this problem, but Aleksei is trying to define a general solution - which unfortunately doesn't exist. The basic deadlock scenario is a special variant of the general class initialization deadlock: Thread 1: - loadLibrary - acquire loadLibrary global lock - call JNI_OnLoad - use class Foo (which needs to be loaded and initialized) - block acquiring lock for Foo Thread 2: - Initialize class Foo - Acquire lock for Foo - - call loadLibrary(x) // for any X - block acquiring loadLibrary global lock We can reduce the chance of deadlock by using a per-native-library lock instead of the global loadLibrary lock - which is what Aleksei's initial version did. But we cannot remove all deadlock possibility because we must ensure only one thread can be executing JNI_OnLoad for any given native library. Cheers, David - Regards, Peter On 5/20/21 12:31 AM, David Holmes wrote: On 20/05/2021 2:29 am, Aleksei Voitylov wrote: On Wed, 19 May 2021 16:21:41 GMT, Aleksei Voitylov 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, add tests Dear colleagues, The updated PR addresses review comment regarding ThreadLocal as well as David' concern around the lock being held during JNI_OnLoad/JNI_OnUnload calls, and ensures all lock objects are deallocated. Multiple threads are allowed to enter NativeLibrary.load() to prevent any thread from locking while another thread loads a library. Before the update, there could be a class loading lock held by a parallel capable class loader, which can deadlock with the library loading lock. As proposed by David Holmes, the library loading lock was removed because dlopen/LoadLibrary are thread safe and they maintain internal reference counters on libraries. There's still a lock being held while a pair of containers are read/updated. It's not going to deadlock as there's no lock/wait operation performed while that lock is held. Multiple threads may create their own copies of NativeLibrary object and register it for auto unloading. Tests for auto unloading were added along with the PR update. There are now 3 jtreg tests: - one checks for deadlock, similar to the one proposed by Chris Hegarty - two other tests are for library unload. The major side effect of that multiple threads are allowed to enter is that JNI_OnLoad/JNI_OnUnload may be called multiple (but
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
Hi Aleksei, Are you trying to solve this in principle or do you have a concrete problem at hand which triggers this deadlock? If it is the later, then some rearrangement of code might do the trick... For example, native libraries are typically loaded by a class initializer of some class that is guaranteed to be initialized before the 1st invocation of a native method from such library. But if such class can also be loaded and initialized by some other trigger, deadlock can occur. Best remedy for such situation is to move all native methods to a special class that serves just for interfacing with native code and also contains an initializer that loads the native library and nothing else. Such arrangement would ensure that the order of taking locks is always the same: classLoadingLock -> nativeLibraryLock ... Regards, Peter On 5/20/21 12:31 AM, David Holmes wrote: On 20/05/2021 2:29 am, Aleksei Voitylov wrote: On Wed, 19 May 2021 16:21:41 GMT, Aleksei Voitylov 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, add tests Dear colleagues, The updated PR addresses review comment regarding ThreadLocal as well as David' concern around the lock being held during JNI_OnLoad/JNI_OnUnload calls, and ensures all lock objects are deallocated. Multiple threads are allowed to enter NativeLibrary.load() to prevent any thread from locking while another thread loads a library. Before the update, there could be a class loading lock held by a parallel capable class loader, which can deadlock with the library loading lock. As proposed by David Holmes, the library loading lock was removed because dlopen/LoadLibrary are thread safe and they maintain internal reference counters on libraries. There's still a lock being held while a pair of containers are read/updated. It's not going to deadlock as there's no lock/wait operation performed while that lock is held. Multiple threads may create their own copies of NativeLibrary object and register it for auto unloading. Tests for auto unloading were added along with the PR update. There are now 3 jtreg tests: - one checks for deadlock, similar to the one proposed by Chris Hegarty - two other tests are for library unload. The major side effect of that multiple threads are allowed to enter is that JNI_OnLoad/JNI_OnUnload may be called multiple (but same) number of times from concurrent threads. In particular, the number of calls to JNI_OnLoad must be equal to the number of calls to JNI_OnUnload after the relevant class loader is garbage collected. This may affect the behaviour that relies on specific order or the number of JNI_OnLoad/JNI_OnUnload calls. The current JNI specification does not mandate how many times JNI_OnLoad/JNI_OnUnload are called. Also, we could not locate tests in jck/jtreg/vmTestbase that would rely on the specific order or number of calls to JNI_OnLoad/JNI_OnUnload. But you can't make such a change! That was my point. To fix the deadlock we must not hold a lock. But we must ensure only a single call to JNI_OnLoad is possible. It is an unsolvable problem with those constraints. You can't just change the behaviour of JNI_OnLoad like that. David - If this is really a problem that several people are facing, then perhaps a change in the API could solve it.
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
On 20/05/2021 2:29 am, Aleksei Voitylov wrote: On Wed, 19 May 2021 16:21:41 GMT, Aleksei Voitylov 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, add tests Dear colleagues, The updated PR addresses review comment regarding ThreadLocal as well as David' concern around the lock being held during JNI_OnLoad/JNI_OnUnload calls, and ensures all lock objects are deallocated. Multiple threads are allowed to enter NativeLibrary.load() to prevent any thread from locking while another thread loads a library. Before the update, there could be a class loading lock held by a parallel capable class loader, which can deadlock with the library loading lock. As proposed by David Holmes, the library loading lock was removed because dlopen/LoadLibrary are thread safe and they maintain internal reference counters on libraries. There's still a lock being held while a pair of containers are read/updated. It's not going to deadlock as there's no lock/wait operation performed while that lock is held. Multiple threads may create their own copies of NativeLibrary object and register it for auto unloading. Tests for auto unloading were added along with the PR update. There are now 3 jtreg tests: - one checks for deadlock, similar to the one proposed by Chris Hegarty - two other tests are for library unload. The major side effect of that multiple threads are allowed to enter is that JNI_OnLoad/JNI_OnUnload may be called multiple (but same) number of times from concurrent threads. In particular, the number of calls to JNI_OnLoad must be equal to the number of calls to JNI_OnUnload after the relevant class loader is garbage collected. This may affect the behaviour that relies on specific order or the number of JNI_OnLoad/JNI_OnUnload calls. The current JNI specification does not mandate how many times JNI_OnLoad/JNI_OnUnload are called. Also, we could not locate tests in jck/jtreg/vmTestbase that would rely on the specific order or number of calls to JNI_OnLoad/JNI_OnUnload. But you can't make such a change! That was my point. To fix the deadlock we must not hold a lock. But we must ensure only a single call to JNI_OnLoad is possible. It is an unsolvable problem with those constraints. You can't just change the behaviour of JNI_OnLoad like that. David - Thank you Alan Bateman, David Holmes and Chris Hegarty for your valuable input. - PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
On Wed, 19 May 2021 16:21:41 GMT, Aleksei Voitylov 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, add tests Dear colleagues, The updated PR addresses review comment regarding ThreadLocal as well as David' concern around the lock being held during JNI_OnLoad/JNI_OnUnload calls, and ensures all lock objects are deallocated. Multiple threads are allowed to enter NativeLibrary.load() to prevent any thread from locking while another thread loads a library. Before the update, there could be a class loading lock held by a parallel capable class loader, which can deadlock with the library loading lock. As proposed by David Holmes, the library loading lock was removed because dlopen/LoadLibrary are thread safe and they maintain internal reference counters on libraries. There's still a lock being held while a pair of containers are read/updated. It's not going to deadlock as there's no lock/wait operation performed while that lock is held. Multiple threads may create their own copies of NativeLibrary object and register it for auto unloading. Tests for auto unloading were added along with the PR update. There are now 3 jtreg tests: - one checks for deadlock, similar to the one proposed by Chris Hegarty - two other tests are for library unload. The major side effect of that multiple threads are allowed to enter is that JNI_OnLoad/JNI_OnUnload may be called multiple (but same) number of times from concurrent threads. In particular, the number of calls to JNI_OnLoad must be equal to the number of calls to JNI_OnUnload after the relevant class loader is garbage collected. This may affect the behaviour that relies on specific order or the number of JNI_OnLoad/JNI_OnUnload calls. The current JNI specification does not mandate how many times JNI_OnLoad/JNI_OnUnload are called. Also, we could not locate tests in jck/jtreg/vmTestbase that would rely on the specific order or number of calls to JNI_OnLoad/JNI_OnUnload. Thank you Alan Bateman, David Holmes and Chris Hegarty for your valuable input. - PR: https://git.openjdk.java.net/jdk/pull/3976
Re: RFR: 8266310: deadlock while loading the JNI code [v2]
> 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, add tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/3976/files - new: https://git.openjdk.java.net/jdk/pull/3976/files/113ee0f3..a0b2163a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3976=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3976=00-01 Stats: 560 lines in 9 files changed: 441 ins; 41 del; 78 mod Patch: https://git.openjdk.java.net/jdk/pull/3976.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3976/head:pull/3976 PR: https://git.openjdk.java.net/jdk/pull/3976
