The change > On Dec 16, 2015, at 11:47 AM, Roger Riggs <roger.ri...@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 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? 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 >>>>>>>> >>>>>> >>>>> >>>> >>> >> >