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

[David Holmes]

Corrections ...

On 27/09/2017 2:36 PM, David Holmes wrote:
> On 27/09/2017 1:36 PM, mandy chung wrote:
>> On 9/26/17 7:35 PM, David Holmes wrote:
>>> On 27/09/2017 12:11 PM, mandy chung wrote:
>>>> On 9/26/17 7:06 PM, David Holmes wrote:
>>>>>
>>>>>> 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?? 
>>>>
>>>> No.  The test reveals a bug in JNI_FindClass that uses a class 
>>>> loader being finalized as the context when NativeLibrary is the caller.
>>>>> And you want to change the FindClass spec to make it clear the test 
>>>>> is what needs to be changed?
>>>> No.   It is a bug in the hotspot implementation.   The JNI spec says 
>>>> that the context of JNI_OnUnload being called is unknown. The 
>>>> hotspot implementation of FindClass uses the class loader associated 
>>>> with that native library as the context when invoked from 
>>>> JNI_OnUnload which is wrong.
>>>
>>> I'm not sure I agree it is wrong. As I've said elsewhere there's a 
>>> good chance that if you are trying to load classes via FindClass as 
>>> part of a unload hook (which implies you are using custom 
>>> classloaders), then it may be only the current loader or a parent 
>>> (still custom) can load that class. But we're on the fringe of 
>>> realistic expectations here as the context is specified as being 
>>> "unknown".
>>>
>> For a native unload hook to access some class defined by this class 
>> loader, definitely it should not write to any fields since the class 
>> and class loader are not strongly reachable.   Reading the current 
>> state stored in the class can be done by writing to the native fields.
> 
> Yes that is a good point - but as the spec says due to the unknown 
> context the hook has to be very careful about what it tries to do. I 
> agree it is doubtful that anyone can, or should, be relying on the 
> direct use of the classloader that has become unreachable, but ...
> 
>> I'd like to know what other use cases that FindClass must ressurrect a 
>> class defined by this class loader or find a class defined by its 
>> ancestor if you have any in mind that the existing code can't be 
>> replaced due to the proposed change.
> 
> ... I can easily imagine a subsystem that runs under a custom loader and 
> which then instantiates further execution contexts (per connection for 
> example) each with their own classloader and which can be reclaimed 
> after the request is complete. I can then easily imagine that they use 
> an unload hook to record statistics about native library use, and that 
> the statistics classes are in the top-level custom loader, and not 
> locatable from the system loader.
> 
> While the spec makes no guarantees this will work it only says 
> programmers "should be conservative in their use of VM services" which 
> strongly suggests to me a "try it and see if it works" approach. In the 
> current code while loading from the loader being reclaimed is highly 
> dubious, delegating through that loader seems fairly reasonable to me.
> 
>>> That said given the spec says "unknown" the behaviour of the VM could 
>>> change and still be in spec.
>>>
>>> I presume that when using a cleaner the current classloader that 
>>> would be used by FindClass is the system loader? Hence the observed 
>>> behaviour of FindClass "changes" if you switch to the cleaner from 
>>> the finalizer - and can't be reverted to the old behaviour by using a 
>>> command-line flag. Hence if we want to be able to revert we have to 
>>> do that in a FindClass-only change first. Then drop-in the cleaner 
>>> update and remove the flag.
>>>
>>>> I will file a separate JBS issue to separate this JNI bug.
>>>
>>> Okay. I see this as a RFE not a bug per-se: change from "unknown 
>>> context" to a specific well known context.
>> This case is arguable whether it's considered as a RFE or a bug 
>> because the current spec of JNI_OnUnload and JNI_FindClass are not 
>> aligned.  I lean toward a bug.    The bottom line:  do you agree with 
>> this proposed JNI spec change?
> 
> I don't think the spec _has_ to change because I disagree that there is 
> a misalignment between JNI_OnUnload and JNI_FindClass. FindClass clearly 
> states it uses the current loader or else the system loader if there is 

That is not accurate - sorry. FindClass doesn't actually address the 
possibility of being called via these load/unload hooks. See more below.

> no notion of a current loader. OnUnload says it runs in an unknown 
> context, so you don't know what the current loader may be, or even if 
> there is one. But regardless a call to FindClass from OnUnload should 
> use the current loader if it exists, or else the system loader. The fact 
> it may be dubious to use the current loader when it is itself in the 
> process of being unloaded does not impinge on the voracity of the spec 
> in my opinion.
> 
> So you can change to using a Cleaner instead of a finalizer and while it 
> will behave differently, that change in behaviour does not violate the 
> spec in any way - again in my opinion.
> 
> Now if you want to pave the way for a future switch to Cleaner by 
> changing the spec for JNI_OnUnload such that it must be executed in a 
> context where (equivalently) there either is no current loader or the 
> current loader is the system loader, then I do not oppose that. But the 
> only purpose that serves is to allow a migration path to the new 
> behaviour - and then forever locks us in.
> 
> Note however I would not want to see the implementation of FindClass 
> having to special case this - I would hope it just happens naturally if 
> the Cleaner thread reports the current class loader as the system 
> loader. Does it?

I missed the fact that we already special case this for JNI_OnLoad and 
JNI_OnUnload. I would have thought that in the OnLoad case we would find 
the classloader of the class loading the native library without any need 
to resort to the NativeLibrary support code in ClassLoader. I guess that 
this:

   // Find calling class
   Klass* k = thread->security_get_caller_class(0);

does not find the "caller" that I would have expected, but instead finds 
java.lang.System because we're executing System.loadLibrary - and hence 
finds the boot loader not the actual loader required.

But the fact we jump through all these hoops is in itself questionable 
because the specification for JNI_FindClass does not indicate this will 
happen. It only accounts for two cases:

1. A JNI call from a declared native method - which uses the loader of 
the class that defines the method

2. A JNI call "through the Invocation Interface" which I interpret as 
being a JNI call from C code, from an attached thread, with no Java 
frames on the stack. In which case the system loader is used.

A call from JNI_OnLoad (or OnUnload) does not, to me, fit either of 
these cases; nor does JNI_OnLoad say anything about the context in which 
it executes. So it seems we have presumed that this case should mean 
"use the loader of the class which loaded the native library". A very 
reasonable approach, but not one defined by the specification as far as 
I can see. But given this, it is not unreasonable to also use the same 
interpretation for JNI_OnUnload.

So there is a gap in the specification regarding the execution context 
of the library lifecycle function hooks - other than onUnload being an 
"unknown context".

David
-----

> 
> Thanks,
> David
> 
>>
>> Mandy