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

[Roger Riggs]

Hi Peter,

It was a bit more involved than I expected, mostly in the tests to make 
this change.

Is this what you expected?  (just the deltas, I'll merge the patches 
before pushing).

http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696-no-clear/

Thanks, Roger



On 12/15/2015 6:01 PM, Peter Levart wrote:
>
>
> On 12/15/2015 11:48 PM, Roger Riggs wrote:
>> Hi Peter,
>>
>> That will break up clearing the ref when the Cleanable is explicitly 
>> cleaned.
>> Reference.clear() needs to be called from Cleanable.clean().
>
> From PhantomCleanable (the superclass of PhantomCleanableRef):
>
>  253         @Override
>  254         public final void clean() {
>  255             if (remove()) {
>  256                 super.clear();
>  257                 performCleanup();
>  258             }
>  259         }
>  260
>  261         /**
>  262          * Unregister this PhantomCleanable and clear the reference.
>  263          * Due to inherent concurrency, {@link #performCleanup()} 
> may still be invoked.
>  264          */
>  265         @Override
>  266         public final void clear() {
>  267             if (remove()) {
>  268                 super.clear();
>  269             }
>  270         }
>
>
> ... clean() calls super.clear(), which is "invokespecial" (not a 
> virtual dispatch).
>
>
> Regards, Peter
>
>>
>> it might be nice to block that but to do so we'd need to go back to 
>> separate objects
>> for the Reference and the Cleanable and we worked hard to get to a 
>> single object.
>>
>> Roger
>>
>>
>> On 12/15/2015 5:38 PM, Peter Levart wrote:
>>> Hi Roger,
>>>
>>> Just one thing about implementation:
>>>
>>> Since the type exposed to user is Cleaner.Cleanable that has only a 
>>> single method clean(), it would be good if the implementation class 
>>> (CleanerImpl.PhantomCleanableRef) overrode 
>>> CleanerImpl.PhantomCleanable.clear() method and threw 
>>> UnsupportedOperationException, otherwise users will be tempted to 
>>> cast the returned Cleaner.Cleanable to Reference and invoke clear() 
>>> method to de-register cleanup action without invoking it. This is 
>>> the only remaining public Reference method that is not disabled this 
>>> way.
>>>
>>> Regards, Peter
>>>
>>> On 12/09/2015 07:40 PM, Roger Riggs wrote:
>>>> Hi,
>>>>
>>>> The example is revised to caution about inner classes and lambdas.
>>>>
>>>> [1]http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
>>>> [2]http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html
>>>>
>>>> Thanks, Roger
>>>>
>>>> On 12/9/2015 11:04 AM, Peter Levart wrote:
>>>>> Hi Chris,
>>>>>
>>>>> On 12/09/2015 04:03 PM, Chris Hegarty wrote:
>>>>>> Peter,
>>>>>>
>>>>>> On 09/12/15 07:05, Peter Levart wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I think the only way to try to prevent such things is with a good
>>>>>>> example in javadoc that "screams" of possible miss-usages.
>>>>>>>
>>>>>>>
>>>>>>> public static class CleanerExample implements AutoCloseable {
>>>>>>>
>>>>>>>          private static final Cleaner cleaner = ...; // 
>>>>>>> preferably a
>>>>>>> shared cleaner
>>>>>>>
>>>>>>>          private final PrivateNativeResource pnr;
>>>>>>>
>>>>>>>          private final Cleaner.Cleanable cleanable;
>>>>>>>
>>>>>>>          public CleanerExample(args, ...) {
>>>>>>>
>>>>>>>              // prepare captured state as local vars...
>>>>>>>              PrivateNativeResource _pnr = ...;
>>>>>>>
>>>>>>>              this.cleanable = cleaner.register(this, () -> {
>>>>>>>                  // DON'T capture any instance fields with 
>>>>>>> lambda since
>>>>>>> that would
>>>>>>>                  // capture 'this' and prevent it from becoming
>>>>>>
>>>>>> I assume that the WARNING should include anonymous inner classes too
>>>>>> ( which I expect are quite common, though less now with lambda ) ?
>>>>>>
>>>>>> Is "leaking" 'this' in a constructor a potential issue with respect
>>>>>> to the visibility of pnr? As well as causing red-squiggly lines in
>>>>>> the IDE ;-)
>>>>>
>>>>> 'this' only leaks to the 'referent' field of PhantomReference 
>>>>> where by definition is not accessible.
>>>>>
>>>>> 'this' can become phantom-reachable before CleanerExample 
>>>>> constructor ends. But this is harmless, because the code that may 
>>>>> execute at that time does not access the object any more, so the 
>>>>> object may be safely collected.
>>>>>
>>>>> Cleanup action can run at any time after registration even before 
>>>>> CleanerExample constructor ends. But this is harmless too, because 
>>>>> it only accesses PrivateNativeResource which is fully constructed 
>>>>> before registration of cleanup action.
>>>>>
>>>>> I see no issues apart from IDE(s) not seeing no issues.
>>>>>
>>>>> Regards, Peter
>>>>>
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>>
>>>>>>> phantom-reachable!!!
>>>>>>>                  _pnr.close();
>>>>>>>              });
>>>>>>>
>>>>>>>              this.pnr = _pnr;
>>>>>>>          }
>>>>>>>
>>>>>>>          public void close() {
>>>>>>>              cleanable.clean();
>>>>>>>          }
>>>>>>>
>>>>>>>
>>>>>>> Regards, Peter
>>>>>>>
>>>>>
>>>>
>>>
>>
>