JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

[Peter Levart]

On 11/04/2013 05:45 AM, Mandy Chung wrote:
>
> On 11/3/2013 5:32 PM, David Holmes wrote:
>> Hi Mandy,
>>
>> On 2/11/2013 7:11 AM, Mandy Chung wrote:
>>> On 11/1/13 1:37 PM, mark.reinhold at oracle.com wrote:
>>>> 2013/11/1 4:15 -0700, mandy.chung at oracle.com:
>>>>> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/
>>>> Looks good.
>>>>
>>>> Just one question: In Finalizer.java, at line 97 you look up the
>>>> JavaLangAccess object every single time.  Is it worth caching that
>>>> earlier, maybe when the finalize thread starts, since it will never
>>>> change?
>>>
>>> I was expecting that would get optimized during runtime and it's a
>>> simple getter method. It's a good suggestion to cache it at the 
>>> finalize
>>> thread start time and here is the revised webrev:
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.01/
>>
>> I'm missing something basic - how did you get this to compile:
>>
>>    public void invokeFinalize(Object o) throws Throwable {
>>      o.finalize();
>>    }
>>
>> given finalize is protected ??
>>
>
> protected members can be accessed by the same package and subclasses.  
> This is the implementation of JavaLangAccess in java.lang.System that 
> is in the same package as java.lang.Object.
>
>> Also VM.awaitBooted seems inherently risky as a general method as you 
>> would have to make sure that it is never called by the main VM 
>> initialization thread. Perhaps handle this in 
>> sun.misc.SharedSecrets.getJavaLangAccess so it is less 'general'? 
>
> That sounds a good idea.  Let me think about it and get back to this.
>
>> That said I think Peter may be right that there could be races with 
>> agents triggerring explicit finalization requests early in the VM 
>> initialization process - which means any blocking operation dependent 
>> on other parts of the initialization sequence could be problematic.
>
> Hmm... agents calling System.runFinalization during startup - like 
> Alan described, the agent is playing fire.

Hi Mandy,

Isn't System.runFinalization() just a "hint"? Like System.gc() for 
example...

      * <p>
      * Calling this method suggests that the Java Virtual Machine expend
      * effort toward running the <code>finalize</code> methods of objects
      * that have been found to be discarded but whose <code>finalize</code>
      * methods have not yet been run. When control returns from the
      * method call, the Java Virtual Machine has made a *best effort* to
      * complete all outstanding finalizations.
      * <p>

Couldn't the request just be ignored when called before VM.isBooted() ? 
The finalizers will be executed nevertheless asynchronously later by the 
finalizer thread...

Regards, Peter

>
> The potential issue that could happen is that during the VM 
> initialization the heap is so small that triggers GC and also the 
> startup code has finalizers and those objects with finalizers are 
> awaiting for finalization in order for the sufficient memory to be 
> freed up.  The VM initialization couldn't get completed and the 
> Finalizer thread is blocked and thus due to insufficient memory, 
> eventually it would get out of memory.  An agent instrumenting classes 
> early in the startup and creates lots of objects and finalizers, that 
> might also cause problem.
>
> I think it's good to have the secondary finalizer thread to call 
> ensureAccessAvailable (with some modification to ensure jla is 
> initialized).
>
>>
>> Overall I think a safer approach may be to fix the native JNI code so 
>> that if it gets a private version of finalize() it looks up the 
>> method in the superclass.
>
> There is other issue (e.g. static method with same name/descriptor) 
> that JNI_GetMethodID has to resolve.  This will be a bigger change in 
> the VM that probably can't make jdk8.
>
> I think the proposed patch with slight change in the secondary 
> finalizer thread is a relative safe approach (I wil revise the patch 
> and send out another rev tomorrow).
>
> thanks
> Mandy