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

[Roger Riggs]

Hi Kim,

Thanks for the comments:

I updated the webrev[2] and javadoc[1] with the editorial improvements.

On 12/02/2015 06:20 PM, Kim Barrett wrote:
> 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.

Yes, the cleaning functions can be completed by calling clean.

* The Cleaner terminates when it is unreachable and all of the
* registered cleaning functions are complete.

>
> Also, there is the inconsistent terminology between "cleaning
> functions" and "Runnables".
Updated to use cleaning function as a generic; Runnable is retained
as the type of the function.
>
> ------------------------------------------------------------------------------
> 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.
An @implNote clause it provides information about the implementation.
'Typically' reduces the statement to informative.
So no, not a promise.
>
> ------------------------------------------------------------------------------
> 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.
Stylistically, nulls are not usually used as defaults and may obscure
unintentional cases of the caller (a bug).
>
> ------------------------------------------------------------------------------
> 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.
fixed
>
> Similarly for "the list" here:
>
>   118      * A Cleanable is in the list for each Object and for the Cleaner
>   119      * itself.
fixed
>
> ------------------------------------------------------------------------------
> 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).
>
> ------------------------------------------------------------------------------
> 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

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

Thanks, Roger

[1] http://cr.openjdk.java.net/~rriggs/cleaner-doc/
[2] http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/

>
> ------------------------------------------------------------------------------
>