The change
> On Dec 16, 2015, at 11:47 AM, Roger Riggs <[email protected]> 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
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
