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

[Peter Levart]

On 11/05/2013 03:21 AM, Mandy Chung wrote:
> On 11/4/2013 6:03 PM, David Holmes wrote:
>> Hi Mandy,
>>
>> This all seems quite reasonable to me.
>>
>> Two minor nits:
>>
>> 1. The "delay ntil" typo in Finalizer.java is still present.
>>
>
> Missed that :(
>
>> 2. In VM.java. booted need not be volatile now that it is only 
>> accessed within a locked region. Also awaitBooted might as well be 
>> void as it can only ever return true.
>>
>
> Fixed.  Revised webrev at:
> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.03/

Hi Mandy,

Just a nit. You are assigning SharedSecrets.getJavaLangAccess() to a 
local 'jla' variable declared outside Runnables, which makes it 
effectively a final field. You could declare and assign the 'jla' inside 
run() methods of both Runnables, just before the loop - like it is done 
in the FinalizerThread...

Regards, Peter

P.S. I'm curious about the strange seemingly unneeded code fragments in 
FinalizerThread and both Runnables. For example:

  169         forkSecondaryFinalizer(new Runnable() {
  170*private volatile boolean running;*
  171             public void run() {
  172*if (running)*
  173*return;*
  174*running = true;*
  

The FinalizerThread and each Runnable instance is only executed once. 
Why these checks then? Does anybody know?


Regards, Peter

>
> Thanks for the review.
> Mandy
>
>> Thanks,
>> David
>>
>> On 5/11/2013 6:04 AM, Mandy Chung wrote:
>>> Thank you all for the suggestions.
>>>
>>> Before the VM initialization is completed, any agent ought to be 
>>> careful
>>> in what things it can do and what shouldn't do.  I think it's 
>>> reasonable
>>> to ignore System.runFinalization if it's called at early startup phase.
>>> I'm unclear if there is any use case that we really require 
>>> finalization
>>> to happen before VM is booted (I can imagine other problems may 
>>> happen).
>>> I change the runFinalization and runAllFinalizers methods to return if
>>> it's not booted and also change runFinalizer method to take
>>> JavaLangAccess to simplify the synchronization instead of caching it in
>>> the static field.
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.02/
>>>
>>> David - I decided to leave VM.awaitBooted as it is.  Having the 
>>> callsite
>>> to call awaitBooted makes it explicit and clear that it's blocking and
>>> we will keep SharedSecrets.getJavaLangAccess method consistent with the
>>> other SharedSecrets methods that they are all getter methods. If any
>>> future change calls SharedSecrets.getJavaLangAccess before it's booted,
>>> it's a little easier to diagnose (NPE vs hangs).
>>>
>>> Peter - thanks for the ObjectAccess idea.  I don't think supporting
>>> finalization to happen before VM is booted buys us anything and I would
>>> rather keep this change simple.
>>>
>>> Mandy
>