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

[David Holmes]

On 27/09/2017 11:32 AM, mandy chung wrote:
> On 9/26/17 5:34 PM, David Holmes wrote:
>> Hi Mandy,
>>
>> On 27/09/2017 3:27 AM, mandy chung wrote:
>>>
>>>
>>> On 9/25/17 11:37 PM, Kim Barrett wrote:
>>>> Thanks for dealing with this.
>>>
>>> I'm all for eliminating the finalizers in the JDK.  Looking forward 
>>> to having a finalizer-free JDK.
>>>> ============================================================================== 
>>>>
>>>> src/java.base/share/native/libjava/ClassLoader.c
>>>>   415 
>>>> Java_java_lang_ClassLoader_00024NativeLibrary_00024Unloader_unload
>>>>   416 (JNIEnv *env, jobject this, jstring name, jboolean isBuiltin, 
>>>> jlong address)
>>>>
>>>> With this change, the "this" argument is no longer used.
>>>>
>>>> Because of this, the native function could be a static member function
>>>> of the new Unloader class, or could (I think) still be a (now static)
>>>> member function of NativeLibrary.  The latter would not require a name
>>>> change, only a signature change.  But I don't really care which class
>>>> has the method.
>>> Yes it can be a static method.
>>>> ============================================================================== 
>>>>
>>>> src/java.base/share/classes/java/lang/ClassLoader.java
>>>> 2394         public void register() {
>>>>                   [...]
>>>> 2406                 // register the class loader for cleanup when 
>>>> unloaded
>>>> 2407                 if (loader != getBuiltinPlatformClassLoader() &&
>>>> 2408                     loader != getBuiltinAppClassLoader()) {
>>>> 2409                     CleanerFactory.cleaner()
>>>> 2410                         .register(loader, new Unloader(name, 
>>>> handle, isBuiltin));
>>>> 2411                 }
>>>>
>>>> Can anything before the cleanup registration throw?
>>>
>>> No within the register method.  The builtin loaders are created early 
>>> during startup.  I shall make sure that System::loadLibrary is not 
>>> called during init phase 1.
>>>> If so, do we leak
>>>> because we haven't registered the cleanup yet?  And what if the
>>>> registration itself fails to complete?
>>>
>>> If Cleaner::register fails, it should throw an exception. Otherwise, 
>>> the registration should complete.
>>>> ============================================================================== 
>>>>
>>>> src/hotspot/share/prims/jni.cpp
>>>>   405     // Special handling to make sure JNI_OnLoad are executed 
>>>> in the correct class context.
>>>>
>>>> s/are executed/is executed/
>>>>
>>> Will fix it.
>>>
>>> I'm considering to separate the JNI_FindClass change to target 18.3 
>>> and provide a flag to restore the current behavior so that it may 
>>> help existing code to identify its dependency on the current behavior 
>>> and give time to migrate.  Then target the finalizer to Cleaner 
>>> change in 18.9.
>>>
>>> It's unknown to us how many existing native libraries are impacted by 
>>> this change (calling FindClass from JNI_OnUnload to load classes 
>>> visible the class loader being unloaded).   I suspect it's rare. If 
>>> FindClass is called when the native library is being unloaded and 
>>> fail to find the class due to this change, it is not hard to find out 
>>> but the code might crash if it does not handle the class not found 
>>> case properly.
>>>
>>> Any opinion?
>>
>> Yes :) I agree with being conservative here. We don't know how this 
>> may be being used. But I'm still not completely clear how the 
>> FindClass change is tied to the switch to Cleaner ??
> It is not tied with the Cleaner change.  Instead, the FindClass bug 
> blocks the finalizer to Cleaner change.
> 
> FindClass bug is uncovered when I implemented the change from finalizer 
> to Cleaner (or phantom reference).   There is a test calling FindClass 
> to look for a class defined by the class loader being unloaded, say L. L 
> is not Gc'ed and so FindClass successfully finds the class (which 
> resurrect the class loader which was marked finalizable).
> 
> Is that clearer?

So the issue is only that this test breaks?? And you want to change the 
FindClass spec to make it clear the test is what needs to be changed?

David

> Mandy