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

[Kim Barrett]

On Dec 2, 2015, at 3:23 PM, Roger Riggs <roger.riggs at oracle.com> wrote:
> 
> Please review the java.lang.ref.Cleaner and tests following the recommendation to simplify the public
> interface to support only phantom cleanup.
> 
> Webrev:
>  http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
> 
> Javadoc:
>   http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html

------------------------------------------------------------------------------ 
src/java.base/share/classes/java/lang/ref/Cleaner.java 
 109      * The Cleaner terminates when it is unreachable and all of the objects
 110      * registered are unreachable and corresponding cleaning functions are complete.
and
 131      * The Cleaner terminates when it is unreachable and all of the objects
 132      * registered are unreachable and corresponding Runnables are complete.

Cleaner termination does not require registered objects to be
unreachable, only that the registered cleaning functions are
complete.  A completed call to clean() on an associated Cleanable also
counts as a cleaning function completion, irrespective of whether the
associated object is unreachable.

Also, there is the inconsistent terminology between "cleaning
functions" and "Runnables".

------------------------------------------------------------------------------ 
src/java.base/share/classes/java/lang/ref/Cleaner.java 
 134      * Typically, only a single thread is requested from the ThreadFactory.

Is that an actual promise?  If not, it might be better to not say
anything at all.

------------------------------------------------------------------------------ 
src/java.base/share/classes/java/lang/ref/Cleaner.java 
 140     public static Cleaner create(ThreadFactory threadFactory) {
 141         Objects.requireNonNull(threadFactory, "threadFactory");

Is there a reason to require threadFactory to be non-null?  This means
that if a caller obtains a factory by some means that might return
null, the caller needs to conditionally select which create call to
make.

------------------------------------------------------------------------------ 
src/java.base/share/classes/jdk/internal/misc/CleanerImpl.java
 117      * Process queued Cleanables as long as the cleanableList is not empty.

There are multiple lists of cleanables; all must be empty.

Similarly for "the list" here:

 118      * A Cleanable is in the list for each Object and for the Cleaner
 119      * itself.

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

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

[Applies to all three of {Soft,Weak,Phantom}Cleanable.]

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

------------------------------------------------------------------------------