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