Review Request JDK-8164512: Replace ClassLoader use of finalizer with phantom reference to unload native library

[mandy chung]

On 10/4/17 6:21 PM, David Holmes wrote:
> Hi Mandy
>
> On 5/10/2017 9:24 AM, mandy chung wrote:
>> Updated webrev:
>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.01/
>>
>> JNI FindClass change has been separated from this patch [1]. I made 
>> further clean up to the NativeLibrary implementation and replaced the 
>> use of Vector/Stack.  I also added a native test to verify the native 
>> library can be unloaded and reloaded.
>>
>> Summary: The patch replaces the ClassLoader use of finalizer with 
>> phantom reference, specifically Cleaner, for unloading native 
>> libraries.  It registers the class loader for cleanup only if it's 
>> not a builtin class loader which will never be unloaded.
>
> src/java.base/share/classes/java/lang/ClassLoader.java
>
> This comment is not a sentence:
>
> 2442                 /* If the library is being loaded (must be by the 
> same thread,
> 2443                  * because Runtime.load and Runtime.loadLibrary are
> 2444                  * synchronous).
>
This is an existing comment.  I rewrite it in the updated webrev.
> Overall the changes seem fine, but I do have a concern about the use 
> of ConcurrentHashMap. This would seem to be a significant change in 
> the initialization order - 

ConcurrentHashMap is referenced in ClassLoader but actually not loaded 
until later.  So it does change the initialization order.   I now change 
the implementation to lazily create CHM.  This saves the footprint when 
a class loader does not load any native library.

> and I'm wondering how libjava itself actually gets loaded ??
>

  os::native_java_library() which is called by JDK_Version_init() at VM 
creation time.

> ---
>
> I assume the tests will be updated to use JNI_VERSION_10, assuming you 
> push the FindClass changes first?

JDK-8188052 will be pushed first to jdk10/hs.  This fix will have to 
wait until JDK-8188052 is integrated to jdk10/master.   I will update 
the test to use JNI_VERSION_10 before I push.

>
> libnativeLibraryTest.c:
>
> Not sure I see the point in the FindClass("java/lang/ClassLoader") as 
> if it fails to find it then surely it already failed to find 
> "java/lang/Error" and you'll possibly crash trying to throw.
>
I agree that should be cleaned up.  I took out 
FindClass("java/lang/ClassLoader") case.
> NativeLibraryTest.java:
>
> Not clear how/if this actually verifies any unloading actually takes 
> place? 

If the native library is not unloaded, p.Test.<clinit> will fail when 
reloading the native library.  I think this is adequate. I added further 
checks to verify the number of times the native library is unloaded as 
you suggest below.

> Also if the OnUnload throws an error and that happens in the Cleaner 
> thread, how would that exception result in the test reporting a 
> failure ??
>

Good point.  Cleaner ignores all exceptions.  I change it to be a fatal 
error.
> I think OnUnload has to update some state stored in the main test 
> class so that it can confirm the unloading occurred successfully, or not.
>

I added this per your suggestion to enhance the verification.

Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.02/

Mandy