Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 10/11/19 2:35 AM, Anton Kozlov wrote: On 11.10.2019 00:28, Mandy Chung wrote: Since the method throws Exception, this try-catch block is not needed. The start year of the copyright in the new test files should be 2019. Thanks! I preserved Amazon's copyright year as 2018, as I derived the file from JDK-8194653 initially ([1]) [1]: https://bugs.openjdk.java.net/secure/attachment/79869/JDK-8194653-Deadlock-involving-FileSystems.patch Ah, I see. Otherwise, looks good. Is there anyone sponsoring this patch for you? If not, I can do that. Not yet. It would be great if you sponsor the patch. Updated webrev: http://cr.openjdk.java.net/~akozlov/8231584/webrev.05/ Pushed. Mandy
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 11.10.2019 00:28, Mandy Chung wrote: > Since the method throws Exception, this try-catch block is not needed. > > The start year of the copyright in the new test files should be 2019. Thanks! I preserved Amazon's copyright year as 2018, as I derived the file from JDK-8194653 initially ([1]) [1]: https://bugs.openjdk.java.net/secure/attachment/79869/JDK-8194653-Deadlock-involving-FileSystems.patch > Otherwise, looks good. > Is there anyone sponsoring this patch for you? If not, I can do that. Not yet. It would be great if you sponsor the patch. Updated webrev: http://cr.openjdk.java.net/~akozlov/8231584/webrev.05/ Thanks, Anton
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 10/10/19 9:54 AM, Anton Kozlov wrote:
Updated review is:
http://cr.openjdk.java.net/~akozlov/8231584/webrev.04/
Nit:
112 try {
113 loader = new TestClassLoader();
114 } catch (MalformedURLException e) {
115 throw new RuntimeException(e);
116 }
Since the method throws Exception, this try-catch block is not needed.
The start year of the copyright in the new test files should be 2019.
Otherwise, looks good.
Is there anyone sponsoring this patch for you? If not, I can do that.
thanks
Mandy
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
Hi Mandy,
On 10.10.2019 08:00, Mandy Chung wrote:
> 2014 static synchronized void initLibraryPaths() {
> This does not need synchronized since it's still during phase 1 before other
> thread can execute java code.
Thanks! I missed this
> LoadLibraryTest.java
> - please add @bug 8231584
> - there are places silently catching the checked exception
> e.g. line 62, 97, 117, 140, 157. I suggest throw an unchecked
> exception instead to help diagnosis in case the test fails.
>
> 121 return defineClass(null, b, 0, b.length);
>
> - it should pass name instead of null
> - in fact, TestClassLoader only needs to delegate the parent classloader
> as it doesn't have any .class to find. So no need to override findClass.
-- fixed as well
Updated review is:
http://cr.openjdk.java.net/~akozlov/8231584/webrev.04/
Thanks,
Anton
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 10/9/19 9:24 AM, Anton Kozlov wrote:
Updated webrev with fixes:
http://cr.openjdk.java.net/~akozlov/8231584/webrev.03/
I like this version. Some minor comments:
2014 static synchronized void initLibraryPaths() { This does not need
synchronized since it's still during phase 1 before other thread can
execute java code.
LoadLibraryTest.java
- please add @bug 8231584
- there are places silently catching the checked exception
e.g. line 62, 97, 117, 140, 157. I suggest throw an unchecked
exception instead to help diagnosis in case the test fails.
121 return defineClass(null, b, 0, b.length);
- it should pass name instead of null
- in fact, TestClassLoader only needs to delegate the parent classloader
as it doesn't have any .class to find. So no need to override findClass.
Thanks
Mandy
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
Hi Mandy,
thanks for the review!
Updated webrev with fixes:
http://cr.openjdk.java.net/~akozlov/8231584/webrev.03/
Few notes below:
On 08.10.2019 01:20, Mandy Chung wrote:
> I think another way doing it is to initialize ClassLoader.sys_paths and
> usr_paths fields at System::initPhase1. These static fields can be
> initialized after the library path system properties are set and before
> calling VM.initLevel(1). ClassLoader::loadLibrary can assert if sys_path
> and usr_paths are non-null.
Agree with this. A more natural place would be phase3, where system class
loader is initialized. But something from phase2 loads a library.
> I prefer to name the test and directory with the API it verifies. For
> example rename the directory to loadLibrary and rename the test files to
Renamed, full path to the test is
java/lang/Runtime/loadLibrary/LoadLibraryTest, as the issue involves
Runtime.loadLibrary in first place.
> 70 static void exitPassed() {
> 71 System.exit(95);
> 72 }
>
> 94 // Finish the test
> 95 exitPassed();
>
>
> Is there a better way to finish up the test? someLibrary does not exist.
> Can it handshake by setting a global state that Target:: can validate
> and expect UnsatisfiedLinkError to be thrown?
Thanks, changed this way. A mark is stored in TestClassLoader after
findLibrary passed after the point of deadlock. The mark is checked in the end
of the test.
Thanks,
Anton
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 10/6/19 9:23 AM, Anton Kozlov wrote:
Hi Mandy,
On 02.10.2019 01:08, Anton Kozlov wrote:>
On 30.09.2019 21:18, Mandy Chung wrote:
Skimming through the implementation, it seems to me that the
Runtime::loadLibrary0 does not need to be synchronized.
ClassLoader::loadLibrary0 should ensure that a native library of a given name
is loaded and registered only once.
Interesting. I get confused at first by a comment nearby [1]. At first sight,
it assumes that Runtime.loadLibrary0 is synchronized. But it's guarded by a
monitor of loadedLibraryNames.
May be this will be simpler approach.
I cannot find a situation when this will be wrong. All the path from
Runtime.loadLibrary (and Runtime.load) to NativeLibrary.loadLibrary do not need
to be synchronized. Except for ClassLoader.sys_paths and usr_paths static
fields initialization, they should be synchronized.
I think another way doing it is to initialize ClassLoader.sys_paths and
usr_paths fields at System::initPhase1. These static fields can be
initialized after the library path system properties are set and before
calling VM.initLevel(1). ClassLoader::loadLibrary can assert if
sys_path and usr_paths are non-null.
I looked at older releases, OpenJDK 7, 8. Relevant code is pretty much the
same: Runtime.{load,loadLibrary} are synced [1], ClassLoader.loadLibrary
eventually grabs a lock [2] and do bookkeeping for loading a native library
only once.
So, if the Runtime.loadLibrary synchronization is redundant for JDK14, then
it's so for older jdks, from the very beginning of _Open_JDK. I'd wish someone
commented why is it there. Is it redundant since then.
The Runtime.{load,loadLibrary} methods synchronize on the Runtime
instance since day 1. The ClassLoader and NativeLibrary implementation
have been changed over time and it looks like this redundant
synchronization is just unnoticed.
I've updated the webrev with this approach and changed the comment. Link is at
the bottom.
:
Updated webrev:
http://cr.openjdk.java.net/~akozlov/8231584/webrev.02/
I prefer to name the test and directory with the API it verifies. For
example rename the directory to loadLibrary and rename the test files to
LoadLibraryTest.java
src/Target.java
src/Target2.java
66 static void exitFailed() {
67 System.exit(1);
68 }
69
The test should simply throw an exception.
70 static void exitPassed() {
71 System.exit(95);
72 }
94 // Finish the test
95 exitPassed();
Is there a better way to finish up the test? someLibrary does not
exist. Can it handshake by setting a global state that Target::
can validate and expect UnsatisfiedLinkError to be thrown?
103 public Class findClass(String name) throws ClassNotFoundException {
use generic Class
The threads should also be daemon threads so that the VM will terminate
if they are alive.
Mandy
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
Hi Mandy,
On 02.10.2019 01:08, Anton Kozlov wrote:>
> > On 30.09.2019 21:18, Mandy Chung wrote:
>> Skimming through the implementation, it seems to me that the
>> Runtime::loadLibrary0 does not need to be synchronized.
>> ClassLoader::loadLibrary0 should ensure that a native library of a given
>> name is loaded and registered only once.
>
> Interesting. I get confused at first by a comment nearby [1]. At first sight,
> it assumes that Runtime.loadLibrary0 is synchronized. But it's guarded by a
> monitor of loadedLibraryNames.
>
> May be this will be simpler approach.
I cannot find a situation when this will be wrong. All the path from
Runtime.loadLibrary (and Runtime.load) to NativeLibrary.loadLibrary do not need
to be synchronized. Except for ClassLoader.sys_paths and usr_paths static
fields initialization, they should be synchronized.
I looked at older releases, OpenJDK 7, 8. Relevant code is pretty much the
same: Runtime.{load,loadLibrary} are synced [1], ClassLoader.loadLibrary
eventually grabs a lock [2] and do bookkeeping for loading a native library
only once.
So, if the Runtime.loadLibrary synchronization is redundant for JDK14, then
it's so for older jdks, from the very beginning of _Open_JDK. I'd wish someone
commented why is it there. Is it redundant since then.
I've updated the webrev with this approach and changed the comment. Link is at
the bottom.
[1]:
https://hg.openjdk.java.net/jdk6/jdk6/jdk/file/jdk6-b00/src/share/classes/java/lang/Runtime.java#l779
[2]:
https://hg.openjdk.java.net/jdk6/jdk6/jdk/file/jdk6-b00/src/share/classes/java/lang/ClassLoader.java#l1732
On 03.10.2019 01:59, Mandy Chung wrote:
> Just on the test, instead of storing Target and Target2 classfile bytes in
> the source, the test can use jdk.test.lib.compiler.CompilerUtils
Thanks for pointing that out, I've changed the test too.
Updated webrev:
http://cr.openjdk.java.net/~akozlov/8231584/webrev.02/
Thanks,
Anton
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 10/1/19 3:08 PM, Anton Kozlov wrote: New intermediate webrev: (while investigating) http://cr.openjdk.java.net/~akozlov/8231584/webrev.01/ Just on the test, instead of storing Target and Target2 classfile bytes in the source, the test can use jdk.test.lib.compiler.CompilerUtils to compile these two test classes in a separate destination read by the TestClassLoader. Mandy
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
On 30.09.2019 21:18, Mandy Chung wrote: > Skimming through the implementation, it seems to me that the > Runtime::loadLibrary0 does not need to be synchronized. > ClassLoader::loadLibrary0 should ensure that a native library of a given name > is loaded and registered only once. Interesting. I get confused at first by a comment nearby [1]. At first sight, it assumes that Runtime.loadLibrary0 is synchronized. But it's guarded by a monitor of loadedLibraryNames. May be this will be simpler approach. [1]: http://hg.openjdk.java.net/jdk/jdk/file/a7c95e2f8814/src/java.base/share/classes/java/lang/ClassLoader.java#l2471 > Can you explain why you think ClassLoader::findLibrary needs to be called > with a lock? I wanted to save findLibrary behavior. The code seems to be there for a long time. I think there are chances some user classLoader could unintentionally depend on findLibrary being called synchronously. For example, a cache for a file system would need a synchronization in custom findLibrary if the outer lock would be removed. But the lock introduces same deadlock, when class being initialized is loaded from the loader with findLibrary overridden. See the test in updated webrev. Removing synchronization solves the issue. Syncing on loader object fixes the test but only because a classLoader lock is taken when resolve is started. I was unable to find an evidence syncing on the loader would be generally applicable. New intermediate webrev: (while investigating) http://cr.openjdk.java.net/~akozlov/8231584/webrev.01/ I removed the sync around findLibrary to resolve the another deadlock; test is improved. Thanks, Anton
Re: RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
I will need to look at this closer. Skimming through the implementation, it seems to me that the Runtime::loadLibrary0 does not need to be synchronized. ClassLoader::loadLibrary0 should ensure that a native library of a given name is loaded and registered only once. Can you explain why you think ClassLoader::findLibrary needs to be called with a lock? Mandy On 9/27/19 9:37 AM, Anton Kozlov wrote: Hi, I think that JDK-8194653 [0] affects jdk/jdk and it doesn't specific to FileSystems.getDefault. I'm starting a new thread/bug as the original issue marked as resolved ([3]) Ryan, thanks for investigation and providing a test [1]! But the test fails in the same way if the FileSystems.getDefault replaced with anything that calls System.loadLibrary in an initializer. For jdk/jdk it may be jdk.net.ExtendedSocketOptions.SO_FLOW_SLA and the issue will arise. I don't think it's possible to provide a general fix for that. We are unable to prevent a user to take the Runtime's monitor and do initialization and loadLibrary, which make a room for deadlock. Taking the lock is incorrect action at first. We can only prevent doing that by accident. From the original deadlock report [2], initialization and loadLibrary under the monitor caused not by java.* code, but class loader's findLibrary (sun.misc.ExtClassLoader to be specific). A workaround for ExtClassLoader is possible, to ensure all necessary classes are inited before findLibrary called, i.e. in the constructor. But then we should require the same for any custom class loader. Chances that it would be implemented correctly are not high since even ExtClassLoader is flawed. I propose to call a ClassLoader.findLibrary with Runtime's monitor unlocked: http://cr.openjdk.java.net/~akozlov/8231584/webrev.00 Thanks, Anton [0]: https://bugs.openjdk.java.net/browse/JDK-8194653 [1]: https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059811.html [2]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-January/050819.html [3]: https://bugs.openjdk.java.net/browse/JDK-8231584
RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call
Hi, I think that JDK-8194653 [0] affects jdk/jdk and it doesn't specific to FileSystems.getDefault. I'm starting a new thread/bug as the original issue marked as resolved ([3]) Ryan, thanks for investigation and providing a test [1]! But the test fails in the same way if the FileSystems.getDefault replaced with anything that calls System.loadLibrary in an initializer. For jdk/jdk it may be jdk.net.ExtendedSocketOptions.SO_FLOW_SLA and the issue will arise. I don't think it's possible to provide a general fix for that. We are unable to prevent a user to take the Runtime's monitor and do initialization and loadLibrary, which make a room for deadlock. Taking the lock is incorrect action at first. We can only prevent doing that by accident. >From the original deadlock report [2], initialization and loadLibrary under >the monitor caused not by java.* code, but class loader's findLibrary >(sun.misc.ExtClassLoader to be specific). A workaround for ExtClassLoader is >possible, to ensure all necessary classes are inited before findLibrary >called, i.e. in the constructor. But then we should require the same for any >custom class loader. Chances that it would be implemented correctly are not >high since even ExtClassLoader is flawed. I propose to call a ClassLoader.findLibrary with Runtime's monitor unlocked: http://cr.openjdk.java.net/~akozlov/8231584/webrev.00 Thanks, Anton [0]: https://bugs.openjdk.java.net/browse/JDK-8194653 [1]: https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059811.html [2]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-January/050819.html [3]: https://bugs.openjdk.java.net/browse/JDK-8231584
