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

[Roger Riggs]

Hi Mandy,

On 12/16/2015 3:07 PM, Mandy Chung wrote:
> The change
>> On Dec 16, 2015, at 11:47 AM, Roger Riggs <Roger.Riggs at oracle.com> 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/
>>
> This change looks good.  Having clear() throwing UOE is right thing to do and prevent user code to cast and call Reference::clear.
>
> Nit: There is no return value and this @return is not needed.
>    * @return does not return
right
>
> In the test:
>   228             Object o = r.get();
>   229             if (!(r instanceof PhantomReference) && !cleaned) {
>   230                 Reference<?> expectedRef = test.getRef();
>   231                 Assert.assertEquals(expectedRef.get(), o,
>   232                         "Object reference incorrect");
>   233             }
>
> Curious on this test case: Is r.get() calling the overridden get() method that always throws null?
The verifyGetRef is used to test both the subclassable XXXCleanable refs and
the PhantomCleanableRef that is exposed using Cleaner.register.
verifyGetRef can be simplified since it is only used (now) to verify the 
ref before it is cleaned
so get returns non-null, except for phantoms.

Thanks, Roger


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