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

[David Holmes]

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).

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 - and I'm wondering how libjava itself actually 
gets loaded ??

---

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

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.

NativeLibraryTest.java:

Not clear how/if this actually verifies any unloading actually takes 
place? 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 ??

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.

Thanks,
David

> thanks
> Mandy
> [1] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049389.html 
> 
>