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

[Peter Levart]

Hi Roger,

On 12/16/2015 08:47 PM, Roger Riggs wrote:
> 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

Yes, that's what I meant. The method is not final any more, but that's 
OK for now as those classes are encapsulated. If this ever gets exposed 
as a public API, it could be made as public subclasses of 
CleanerImpl.XXXXCleanable classes and those subclasses could make the 
method final.

I see no other issues left besides the reachability races that I talked 
about in previous messages. Do you know if Reference.reachabilityFence 
is being pushed before this API?

Regards, Peter

>
>
>
> 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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>