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

[Kim Barrett]

On Dec 3, 2015, at 4:19 PM, Roger Riggs <roger.riggs at oracle.com> wrote:
> 
>> src/java.base/share/classes/jdk/internal/misc/CleanerImpl.java 
>>  199         PhantomCleanable(CleanerImpl cleanerImpl) {
>>  200             super(null, null);
>> 
>> This feels mildly icky, passing a null referent to a Reference
>> constructor.  (Similarly for the other XxxCleanable classes.)
>> 
>> Also, in this case, passing a null queue to the PhantomReference
>> class.
>> 
>> Both (presently) work, but still...
>> 
> A dummy object could be allocated for the referent but it would have not purpose.
> (and null is allowed).

Yes, I know.  It still feels icky to me.  Oh well.

>> 
>> ------------------------------------------------------------------------------ 
>> src/java.base/share/classes/jdk/internal/misc/CleanerImpl.java 
>>  223         private boolean remove() {
>>  224             PhantomCleanable<?> list = cleanerImpl.phantomCleanableList;
>>  225             synchronized (list) {
>>  226                 if (next != this) {
>> 
>> Shouldn't that be "if (prev != this) {" ?
>> 
>> Insertion is always after the special sentinal, so "prev" should
>> always be non-"this" when in the list, and "this" when out.  But
>> "this" could be the last element in the list, in which case
>> "next == this", even though in the list.
>> 
> Each list is a doubly linked circular list with the list head as a separate distinct ref and is never removed.
> When not in a list prev == next == this;  (except in list root; the list is empty)
> When it is in a list, prev != this && next != this

Drat!  I forgot or failed to notice the lists are circular.  Yesterday seems to have been
a particularly bad code understanding day for me.

>> src/java.base/share/classes/jdk/internal/misc/CleanerImpl.java 
>>  237         /**
>>  238          * Returns true if the list's next reference refers to itself.
>>  239          *
>>  240          * @return true if the list is empty
>>  241          */
>>  242         boolean isListEmpty() {
>>  243             PhantomCleanable<?> list = cleanerImpl.phantomCleanableList;
>>  244             synchronized (list) {
>>  245                 return next == list;
>>  246             }
>>  247         }
>> 
>> Based on the description, one might expect some occurrence of "this"
>> in the body.  This function must only be applied to the special head
>> sentinal, so a pre-condition is "list == this".  Either an assert or
>> some mention of this would make this easier to understand.
>> 
>> [Applies to all three of {Soft,Weak,Phantom}Cleanable.]
>> 
> The implicit reference to 'this' is in via cleanerImpl.
> 
> All of the functions insert, remove, isListEmpty need to lock on the list root
> and explicitly referring to the list root (via the clearerImpl) in each method 
> keeps them aligned.
> IsListEmpty could be recoded to operate only on the distinguished root entry
> but it would be harder to see that the same lock is being used consistently.

I think that, as written, it would return true if applied to the last element of a
non-empty list.  (That never happens at present.)  If the test were
"list == list.next” that (potential) failure mode would be eliminated.  That
would also make the code and the comment line up nicely, though the
comment would then be kind of redundant, as it doesn’t provide any
explanation for why that works.  Your call.

So looks good to me.