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.ri...@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/
------------------------------------------------------------------------------
