RFR: 8231584: Deadlock with ClassLoader.findLibrary and System.loadLibrary call

[Mandy Chung]

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