RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[Peter Levart]

Not so fast, there's more...

On 10/21/2015 01:50 PM, Peter Levart wrote:
> Hi Roger,
>
> I think I have another race ;-)
>
> This time it is more theoretical than practical. If we take a look at 
> say PhantomCleanable constructor:
>
>         public PhantomCleanable(T referent, Cleaner cleaner) {
>             super(Objects.requireNonNull(referent), 
> getCleanerImpl(cleaner).queue);
>             this.cleanerImpl = getCleanerImpl(cleaner);
>             insert();
>         }
>
> ...somewhere in the getCleanerImpl(cleaner) method where 'cleaner' is 
> last dereferenced and before assignment of extracted CleanerImpl to 
> this.cleanerImpl, 'cleaner' is not needed any more. If it is not 
> referenced from anywhere else in the program and JVM can prove it will 
> not be used any more, JVM is free to declare it phantom reachable. The 
> PhantomCleanable that is used to track the 'cleaner' can therefore be 
> enqueued and the cleaner thread can process it, effectively emptying 
> the 'phantomCleanableList' before this PhantomCleanable constructor 
> insert()s itself into the 'phantomCleanableList'. The cleaner thread 
> can therefore exit the loop and terminate prematurely.
>
> The fix would be to reverse the last two statements of the 
> XxxCleanable constructor(s):
>
>         public PhantomCleanable(T referent, Cleaner cleaner) {
>             super(Objects.requireNonNull(referent), 
> getCleanerImpl(cleaner).queue);
>             insert();
>             this.cleanerImpl = getCleanerImpl(cleaner);
>         }
>
> ... or to add a call to the (not yet in jdk9 but coming soon) 
> reachabilityFence():
>
>         public PhantomCleanable(T referent, Cleaner cleaner) {
>             super(Objects.requireNonNull(referent), 
> getCleanerImpl(cleaner).queue);
>             this.cleanerImpl = getCleanerImpl(cleaner);
>             insert();
>             Reference.reachabilityFence(cleaner);
>         }
>

...similar race could be devised around the reachability of 'referent'. 
As soon as it is assigned to the Reference.referent field, it is not 
needed any more, so it can be declared (phantom|weakly|softly)-reachable 
by JVM before the XxxCleanable instance is insert()ed causing it to be 
attempted to be remove()d before it is insert()ed. The full fix 
therefore needs this:

         public PhantomCleanable(T referent, Cleaner cleaner) {
             super(Objects.requireNonNull(referent), 
getCleanerImpl(cleaner).queue);
             this.cleanerImpl = getCleanerImpl(cleaner);
             insert();
             Reference.reachabilityFence(cleaner);
             Reference.reachabilityFence(referent);
         }


Regards, Peter

>
> Regards, Peter
>
> On 10/20/2015 08:28 PM, Roger Riggs wrote:
>> Sorry for the silence, JavaOne preparations and the availability of 
>> folks who wanted
>> to review have stretched things out.
>>
>> The Cleaner API was very simple and saw feature creep as the ideas 
>> for how it might be used
>> were explored.  There are concerns about committing to supporting 
>> subclassable
>> CleanableReferences in all future JDK versions before there had been 
>> a chance to
>> see how if they would be useful and necessary to address the need to 
>> reduce the
>> use of finalization within the OpenJDK and beyond.
>>
>> Recent updates:
>>  - The Cleaner implementation classes and the CleanableReference 
>> abstract classes are
>>    now in the jdk.internal.misc package and are available within the 
>> java.base module.
>>  - The Cleanable.clear method has been dropped; there is no current 
>> use case.
>>    Since the CleanableReferences extend Reference, clear() is 
>> available when subclassing.
>>  - The tests have been extended to cover the exported and internal APIs
>>
>> The Runnable thunk version is very convenient to code but does 
>> transparently create
>> an additional object to hold the bindings.
>> As the Cleaner is applied to the various uses of finalize we'll see 
>> how they would be used
>> and can re-evaluate the exported API needs.
>>
>> Updated Javadoc:
>>   http://cr.openjdk.java.net/~rriggs/cleaner-doc/
>>
>> Updated Webrev:
>>    http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
>>
>> Thanks, Roger
>>
>>
>>
>>
>