--- old/src/java.base/share/classes/java/lang/ref/Cleaner.java 2016-05-15 22:37:38.012950492 +0200 +++ new/src/java.base/share/classes/java/lang/ref/Cleaner.java 2016-05-15 22:37:37.865953023 +0200 @@ -217,7 +217,7 @@ public Cleanable register(Object obj, Runnable action) { Objects.requireNonNull(obj, "obj"); Objects.requireNonNull(action, "action"); - return new CleanerImpl.PhantomCleanableRef(obj, this, action); + return new CleanerImpl.PhantomCleanableImpl(obj, this, action); } /** --- old/src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java 2016-05-15 22:37:38.344944778 +0200 +++ new/src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java 2016-05-15 22:37:38.207947136 +0200 @@ -25,8 +25,11 @@ package jdk.internal.ref; +import jdk.internal.misc.InnocuousThread; + import java.lang.ref.Cleaner; import java.lang.ref.Cleaner.Cleanable; +import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; import java.security.AccessController; import java.security.PrivilegedAction; @@ -34,8 +37,6 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; -import jdk.internal.misc.InnocuousThread; - /** * CleanerImpl manages a set of object references and corresponding cleaning actions. * CleanerImpl provides the functionality of {@link java.lang.ref.Cleaner}. @@ -48,15 +49,13 @@ private static Function cleanerImplAccess = null; /** - * Heads of a CleanableList for each reference type. + * Head of the PhantomCleanableImpl list. */ - final PhantomCleanable phantomCleanableList; - - final WeakCleanable weakCleanableList; - - final SoftCleanable softCleanableList; + final PhantomCleanableImpl phantomCleanableList; - // The ReferenceQueue of pending cleaning actions + /** + * The ReferenceQueue of pending cleaning actions + */ final ReferenceQueue queue; /** @@ -86,9 +85,7 @@ */ public CleanerImpl() { queue = new ReferenceQueue<>(); - phantomCleanableList = new PhantomCleanableRef(); - weakCleanableList = new WeakCleanableRef(); - softCleanableList = new SoftCleanableRef(); + phantomCleanableList = new PhantomCleanableImpl(); } /** @@ -102,9 +99,9 @@ if (getCleanerImpl(cleaner) != this) { throw new AssertionError("wrong cleaner"); } - // schedule a nop cleaning action for the cleaner, so the associated thread - // will continue to run at least until the cleaner is reclaimable. - new CleanerCleanable(cleaner); + // schedule a no-op cleaning action for the cleaner, so the associated + // thread will continue to run at least until the cleaner is reclaimable. + new PhantomCleanableImpl(cleaner, cleaner, null); if (threadFactory == null) { threadFactory = CleanerImpl.InnocuousThreadFactory.factory(); @@ -135,9 +132,7 @@ InnocuousThread mlThread = (t instanceof InnocuousThread) ? (InnocuousThread) t : null; - while (!phantomCleanableList.isListEmpty() || - !weakCleanableList.isListEmpty() || - !softCleanableList.isListEmpty()) { + while (!phantomCleanableList.isListEmpty()) { if (mlThread != null) { // Clear the thread locals mlThread.eraseThreadLocals(); @@ -157,84 +152,107 @@ } /** - * Perform cleaning on an unreachable PhantomReference. + * Perform cleaning on an unreachable PhantomCleanable's referent. */ - public static final class PhantomCleanableRef extends PhantomCleanable { + public static final class PhantomCleanableImpl extends PhantomCleanable { + + /** + * The list of PhantomCleanableImpl; synchronizes insert and remove. + */ + private final PhantomCleanableImpl list; + + /** + * The cleanup action. + */ private final Runnable action; /** - * Constructor for a phantom cleanable reference. + * Links to previous and next in a doubly-linked list. + */ + private PhantomCleanableImpl prev = this, next = this; + + /** + * Constructor for a PhantomCleanableImpl. * @param obj the object to monitor * @param cleaner the cleaner - * @param action the action Runnable + * @param action the action Runnable (or null if no-op) */ - public PhantomCleanableRef(Object obj, Cleaner cleaner, Runnable action) { + public PhantomCleanableImpl(Object obj, Cleaner cleaner, Runnable action) { super(obj, cleaner); + this.list = CleanerImpl.getCleanerImpl(cleaner).phantomCleanableList; this.action = action; + // register this PhantomCleanableImpl instance so it remains strongly + // reachable until cleaned + insert(); + // ensure obj and cleaner remain strongly reachable at least until + // this PhantomCleanableImpl is registered + Reference.reachabilityFence(obj); + Reference.reachabilityFence(cleaner); } /** * Constructor used only for root of phantom cleanable list. */ - PhantomCleanableRef() { + PhantomCleanableImpl() { super(); + this.list = this; this.action = null; } - @Override - protected void performCleanup() { - action.run(); - } - /** - * Prevent access to referent even when it is still alive. - * - * @throws UnsupportedOperationException always + * Insert this PhantomCleanableImpl after the list head. */ - @Override - public Object get() { - throw new UnsupportedOperationException("get"); + private void insert() { + synchronized (list) { + prev = list; + next = list.next; + next.prev = this; + list.next = this; + } } /** - * Direct clearing of the referent is not supported. + * Remove this PhantomCleanableImpl from the list. * - * @throws UnsupportedOperationException always + * @return true if Cleanable was removed or false if not because + * it had already been removed before */ - @Override - public void clear() { - throw new UnsupportedOperationException("clear"); + private boolean remove() { + synchronized (list) { + if (next != this) { + next.prev = prev; + prev.next = next; + prev = this; + next = this; + return true; + } + return false; + } } - } - - /** - * Perform cleaning on an unreachable WeakReference. - */ - public static final class WeakCleanableRef extends WeakCleanable { - private final Runnable action; /** - * Constructor for a weak cleanable reference. - * @param obj the object to monitor - * @param cleaner the cleaner - * @param action the action Runnable + * Returns true if the list's next reference refers to itself. + * + * @return true if the list is empty */ - WeakCleanableRef(Object obj, Cleaner cleaner, Runnable action) { - super(obj, cleaner); - this.action = action; + boolean isListEmpty() { + synchronized (list) { + return list == list.next; + } } /** - * Constructor used only for root of weak cleanable list. + * Unregister this PhantomCleanableImpl and invoke cleanup action, + * ensuring at-most-once semantics. */ - WeakCleanableRef() { - super(); - this.action = null; - } - @Override - protected void performCleanup() { - action.run(); + public final void clean() { + if (remove()) { + super.clear(); + if (action != null) { + action.run(); + } + } } /** @@ -256,58 +274,30 @@ public void clear() { throw new UnsupportedOperationException("clear"); } - } - - /** - * Perform cleaning on an unreachable SoftReference. - */ - public static final class SoftCleanableRef extends SoftCleanable { - private final Runnable action; - - /** - * Constructor for a soft cleanable reference. - * @param obj the object to monitor - * @param cleaner the cleaner - * @param action the action Runnable - */ - SoftCleanableRef(Object obj, Cleaner cleaner, Runnable action) { - super(obj, cleaner); - this.action = action; - } - - /** - * Constructor used only for root of soft cleanable list. - */ - SoftCleanableRef() { - super(); - this.action = null; - } - - @Override - protected void performCleanup() { - action.run(); - } /** - * Prevent access to referent even when it is still alive. + * This method always throws {@link UnsupportedOperationException}. + * Enqueuing details of {@link Cleaner.Cleanable} + * are a private implementation detail. * * @throws UnsupportedOperationException always */ @Override - public Object get() { - throw new UnsupportedOperationException("get"); + public final boolean isEnqueued() { + throw new UnsupportedOperationException("isEnqueued"); } /** - * Direct clearing of the referent is not supported. + * This method always throws {@link UnsupportedOperationException}. + * Enqueuing details of {@link Cleaner.Cleanable} + * are a private implementation detail. * * @throws UnsupportedOperationException always */ @Override - public void clear() { - throw new UnsupportedOperationException("clear"); + public final boolean enqueue() { + throw new UnsupportedOperationException("enqueue"); } - } /** @@ -335,18 +325,4 @@ }); } } - - /** - * A PhantomCleanable implementation for tracking the Cleaner itself. - */ - static final class CleanerCleanable extends PhantomCleanable { - CleanerCleanable(Cleaner cleaner) { - super(cleaner, cleaner); - } - - @Override - protected void performCleanup() { - // no action - } - } } --- old/src/java.base/share/classes/jdk/internal/ref/PhantomCleanable.java 2016-05-15 22:37:38.662939305 +0200 +++ new/src/java.base/share/classes/jdk/internal/ref/PhantomCleanable.java 2016-05-15 22:37:38.525941663 +0200 @@ -26,154 +26,42 @@ package jdk.internal.ref; import java.lang.ref.Cleaner; -import java.lang.ref.Reference; import java.lang.ref.PhantomReference; import java.util.Objects; /** * PhantomCleanable subclasses efficiently encapsulate cleanup state and - * the cleaning action. - * Subclasses implement the abstract {@link #performCleanup()} method + * the cleaning action. Subclasses implement the abstract {@link #clean()} method * to provide the cleaning action. - * When constructed, the object reference and the {@link Cleaner.Cleanable Cleanable} - * are registered with the {@link Cleaner}. - * The Cleaner invokes {@link Cleaner.Cleanable#clean() clean} after the - * referent becomes phantom reachable. + * When constructed, the object reference is registered with the {@link Cleaner}. + * The Cleaner invokes {@link #clean()} after the referent becomes phantom reachable + * if the PhantomCleanable instance is still reachable at that time. + * Subclasses must ensure for PhantomCleanable instance and its associated Cleaner + * to remain reachable if they want to ensure execution of the cleaning action. */ public abstract class PhantomCleanable extends PhantomReference implements Cleaner.Cleanable { /** - * Links to previous and next in a doubly-linked list. - */ - PhantomCleanable prev = this, next = this; - - /** - * The list of PhantomCleanable; synchronizes insert and remove. - */ - private final PhantomCleanable list; - - /** * Constructs new {@code PhantomCleanable} with * {@code non-null referent} and {@code non-null cleaner}. * The {@code cleaner} is not retained; it is only used to - * register the newly constructed {@link Cleaner.Cleanable Cleanable}. + * register the newly constructed {@link Cleaner.Cleanable Cleanable} with + * its ReferenceQueue. * * @param referent the referent to track * @param cleaner the {@code Cleaner} to register with */ - public PhantomCleanable(T referent, Cleaner cleaner) { - super(Objects.requireNonNull(referent), CleanerImpl.getCleanerImpl(cleaner).queue); - this.list = CleanerImpl.getCleanerImpl(cleaner).phantomCleanableList; - insert(); - - // Ensure referent and cleaner remain accessible - Reference.reachabilityFence(referent); - Reference.reachabilityFence(cleaner); + protected PhantomCleanable(T referent, Cleaner cleaner) { + super(Objects.requireNonNull(referent), + CleanerImpl.getCleanerImpl(cleaner).queue); } /** - * Construct a new root of the list; not inserted. + * Constructor for special-purpose instances that don't participate in + * registration protocol. */ PhantomCleanable() { super(null, null); - this.list = this; - } - - /** - * Insert this PhantomCleanable after the list head. - */ - private void insert() { - synchronized (list) { - prev = list; - next = list.next; - next.prev = this; - list.next = this; - } - } - - /** - * Remove this PhantomCleanable from the list. - * - * @return true if Cleanable was removed or false if not because - * it had already been removed before - */ - private boolean remove() { - synchronized (list) { - if (next != this) { - next.prev = prev; - prev.next = next; - prev = this; - next = this; - return true; - } - return false; - } - } - - /** - * Returns true if the list's next reference refers to itself. - * - * @return true if the list is empty - */ - boolean isListEmpty() { - synchronized (list) { - return list == list.next; - } - } - - /** - * Unregister this PhantomCleanable and invoke {@link #performCleanup()}, - * ensuring at-most-once semantics. - */ - @Override - public final void clean() { - if (remove()) { - super.clear(); - performCleanup(); - } - } - - /** - * Unregister this PhantomCleanable and clear the reference. - * Due to inherent concurrency, {@link #performCleanup()} may still be invoked. - */ - @Override - public void clear() { - if (remove()) { - super.clear(); - } - } - - /** - * The {@code performCleanup} abstract method is overridden - * to implement the cleaning logic. - * The {@code performCleanup} method should not be called except - * by the {@link #clean} method which ensures at most once semantics. - */ - protected abstract void performCleanup(); - - /** - * This method always throws {@link UnsupportedOperationException}. - * Enqueuing details of {@link Cleaner.Cleanable} - * are a private implementation detail. - * - * @throws UnsupportedOperationException always - */ - @Override - public final boolean isEnqueued() { - throw new UnsupportedOperationException("isEnqueued"); - } - - /** - * This method always throws {@link UnsupportedOperationException}. - * Enqueuing details of {@link Cleaner.Cleanable} - * are a private implementation detail. - * - * @throws UnsupportedOperationException always - */ - @Override - public final boolean enqueue() { - throw new UnsupportedOperationException("enqueue"); } } --- old/src/java.base/share/classes/jdk/internal/ref/SoftCleanable.java 2016-05-15 22:37:38.995933573 +0200 +++ new/src/java.base/share/classes/jdk/internal/ref/SoftCleanable.java 2016-05-15 22:37:38.869935742 +0200 @@ -26,154 +26,34 @@ package jdk.internal.ref; import java.lang.ref.Cleaner; -import java.lang.ref.Reference; import java.lang.ref.SoftReference; import java.util.Objects; /** * SoftCleanable subclasses efficiently encapsulate cleanup state and - * the cleaning action. - * Subclasses implement the abstract {@link #performCleanup()} method + * the cleaning action. Subclasses implement the abstract {@link #clean()} method * to provide the cleaning action. - * When constructed, the object reference and the {@link Cleaner.Cleanable Cleanable} - * are registered with the {@link Cleaner}. - * The Cleaner invokes {@link Cleaner.Cleanable#clean() clean} after the - * referent becomes softly reachable. + * When constructed, the object reference is registered with the {@link Cleaner}. + * The Cleaner invokes {@link #clean()} after the referent becomes softly reachable + * if the SoftCleanable instance is still reachable at that time. + * Subclasses must ensure for SoftCleanable instance and its associated Cleaner + * to remain reachable if they want to ensure execution of the cleaning action. */ public abstract class SoftCleanable extends SoftReference implements Cleaner.Cleanable { /** - * Links to previous and next in a doubly-linked list. - */ - SoftCleanable prev = this, next = this; - - /** - * The list of SoftCleanable; synchronizes insert and remove. - */ - private final SoftCleanable list; - - /** - * Constructs new {@code SoftCleanableReference} with + * Constructs new {@code SoftCleanable} with * {@code non-null referent} and {@code non-null cleaner}. - * The {@code cleaner} is not retained by this reference; it is only used - * to register the newly constructed {@link Cleaner.Cleanable Cleanable}. + * The {@code cleaner} is not retained; it is only used to + * register the newly constructed {@link Cleaner.Cleanable Cleanable} with + * its ReferenceQueue. * * @param referent the referent to track * @param cleaner the {@code Cleaner} to register with */ - public SoftCleanable(T referent, Cleaner cleaner) { - super(Objects.requireNonNull(referent), CleanerImpl.getCleanerImpl(cleaner).queue); - list = CleanerImpl.getCleanerImpl(cleaner).softCleanableList; - insert(); - - // Ensure referent and cleaner remain accessible - Reference.reachabilityFence(referent); - Reference.reachabilityFence(cleaner); - } - - /** - * Construct a new root of the list; not inserted. - */ - SoftCleanable() { - super(null, null); - this.list = this; - } - - /** - * Insert this SoftCleanableReference after the list head. - */ - private void insert() { - synchronized (list) { - prev = list; - next = list.next; - next.prev = this; - list.next = this; - } - } - - /** - * Remove this SoftCleanableReference from the list. - * - * @return true if Cleanable was removed or false if not because - * it had already been removed before - */ - private boolean remove() { - synchronized (list) { - if (next != this) { - next.prev = prev; - prev.next = next; - prev = this; - next = this; - return true; - } - return false; - } - } - - /** - * Returns true if the list's next reference refers to itself. - * - * @return true if the list is empty - */ - boolean isListEmpty() { - synchronized (list) { - return list == list.next; - } - } - - /** - * Unregister this SoftCleanable reference and invoke {@link #performCleanup()}, - * ensuring at-most-once semantics. - */ - @Override - public final void clean() { - if (remove()) { - super.clear(); - performCleanup(); - } - } - - /** - * Unregister this SoftCleanable and clear the reference. - * Due to inherent concurrency, {@link #performCleanup()} may still be invoked. - */ - @Override - public void clear() { - if (remove()) { - super.clear(); - } - } - - /** - * The {@code performCleanup} abstract method is overridden - * to implement the cleaning logic. - * The {@code performCleanup} method should not be called except - * by the {@link #clean} method which ensures at most once semantics. - */ - protected abstract void performCleanup(); - - /** - * This method always throws {@link UnsupportedOperationException}. - * Enqueuing details of {@link Cleaner.Cleanable} - * are a private implementation detail. - * - * @throws UnsupportedOperationException always - */ - @Override - public final boolean isEnqueued() { - throw new UnsupportedOperationException("isEnqueued"); - } - - /** - * This method always throws {@link UnsupportedOperationException}. - * Enqueuing details of {@link Cleaner.Cleanable} - * are a private implementation detail. - * - * @throws UnsupportedOperationException always - */ - @Override - public final boolean enqueue() { - throw new UnsupportedOperationException("enqueue"); + protected SoftCleanable(T referent, Cleaner cleaner) { + super(Objects.requireNonNull(referent), + CleanerImpl.getCleanerImpl(cleaner).queue); } } --- old/src/java.base/share/classes/jdk/internal/ref/WeakCleanable.java 2016-05-15 22:37:39.300928323 +0200 +++ new/src/java.base/share/classes/jdk/internal/ref/WeakCleanable.java 2016-05-15 22:37:39.168930595 +0200 @@ -1,5 +1,3 @@ -package jdk.internal.ref; - /* * Copyright (c) 2015, 2016, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. @@ -25,156 +23,38 @@ * questions. */ +package jdk.internal.ref; + import java.lang.ref.Cleaner; -import java.lang.ref.Reference; +import java.lang.ref.PhantomReference; import java.lang.ref.WeakReference; import java.util.Objects; /** * WeakCleanable subclasses efficiently encapsulate cleanup state and - * the cleaning action. - * Subclasses implement the abstract {@link #performCleanup()} method + * the cleaning action. Subclasses implement the abstract {@link #clean()} method * to provide the cleaning action. - * When constructed, the object reference and the {@link Cleaner.Cleanable Cleanable} - * are registered with the {@link Cleaner}. - * The Cleaner invokes {@link Cleaner.Cleanable#clean() clean} after the - * referent becomes weakly reachable. + * When constructed, the object reference is registered with the {@link Cleaner}. + * The Cleaner invokes {@link #clean()} after the referent becomes weakly reachable + * if the WeakCleanable instance is still reachable at that time. + * Subclasses must ensure for WeakCleanable instance and its associated Cleaner + * to remain reachable if they want to ensure execution of the cleaning action. */ public abstract class WeakCleanable extends WeakReference implements Cleaner.Cleanable { /** - * Links to previous and next in a doubly-linked list. - */ - WeakCleanable prev = this, next = this; - - /** - * The list of WeakCleanable; synchronizes insert and remove. - */ - private final WeakCleanable list; - - /** - * Constructs new {@code WeakCleanableReference} with + * Constructs new {@code WeakCleanable} with * {@code non-null referent} and {@code non-null cleaner}. - * The {@code cleaner} is not retained by this reference; it is only used - * to register the newly constructed {@link Cleaner.Cleanable Cleanable}. + * The {@code cleaner} is not retained; it is only used to + * register the newly constructed {@link Cleaner.Cleanable Cleanable} with + * its ReferenceQueue. * * @param referent the referent to track - * @param cleaner the {@code Cleaner} to register new reference with - */ - public WeakCleanable(T referent, Cleaner cleaner) { - super(Objects.requireNonNull(referent), CleanerImpl.getCleanerImpl(cleaner).queue); - list = CleanerImpl.getCleanerImpl(cleaner).weakCleanableList; - insert(); - - // Ensure referent and cleaner remain accessible - Reference.reachabilityFence(referent); - Reference.reachabilityFence(cleaner); - - } - - /** - * Construct a new root of the list; not inserted. - */ - WeakCleanable() { - super(null, null); - this.list = this; - } - - /** - * Insert this WeakCleanableReference after the list head. - */ - private void insert() { - synchronized (list) { - prev = list; - next = list.next; - next.prev = this; - list.next = this; - } - } - - /** - * Remove this WeakCleanableReference from the list. - * - * @return true if Cleanable was removed or false if not because - * it had already been removed before - */ - private boolean remove() { - synchronized (list) { - if (next != this) { - next.prev = prev; - prev.next = next; - prev = this; - next = this; - return true; - } - return false; - } - } - - /** - * Returns true if the list's next reference refers to itself. - * - * @return true if the list is empty - */ - boolean isListEmpty() { - synchronized (list) { - return list == list.next; - } - } - - /** - * Unregister this WeakCleanable reference and invoke {@link #performCleanup()}, - * ensuring at-most-once semantics. - */ - @Override - public final void clean() { - if (remove()) { - super.clear(); - performCleanup(); - } - } - - /** - * Unregister this WeakCleanable and clear the reference. - * Due to inherent concurrency, {@link #performCleanup()} may still be invoked. - */ - @Override - public void clear() { - if (remove()) { - super.clear(); - } - } - - /** - * The {@code performCleanup} abstract method is overridden - * to implement the cleaning logic. - * The {@code performCleanup} method should not be called except - * by the {@link #clean} method which ensures at most once semantics. - */ - protected abstract void performCleanup(); - - /** - * This method always throws {@link UnsupportedOperationException}. - * Enqueuing details of {@link Cleaner.Cleanable} - * are a private implementation detail. - * - * @throws UnsupportedOperationException always - */ - @Override - public final boolean isEnqueued() { - throw new UnsupportedOperationException("isEnqueued"); - } - - /** - * This method always throws {@link UnsupportedOperationException}. - * Enqueuing details of {@link Cleaner.Cleanable} - * are a private implementation detail. - * - * @throws UnsupportedOperationException always + * @param cleaner the {@code Cleaner} to register with */ - @Override - public final boolean enqueue() { - throw new UnsupportedOperationException("enqueue"); + protected WeakCleanable(T referent, Cleaner cleaner) { + super(Objects.requireNonNull(referent), + CleanerImpl.getCleanerImpl(cleaner).queue); } } --- old/test/java/lang/ref/CleanerTest.java 2016-05-15 22:37:39.613922936 +0200 +++ new/test/java/lang/ref/CleanerTest.java 2016-05-15 22:37:39.499924898 +0200 @@ -21,12 +21,23 @@ * questions. */ +import jdk.internal.ref.CleanerFactory; +import jdk.internal.ref.CleanerImpl; +import jdk.internal.ref.PhantomCleanable; +import jdk.internal.ref.SoftCleanable; +import jdk.internal.ref.WeakCleanable; +import jdk.test.lib.Utils; +import org.testng.Assert; +import org.testng.annotations.Test; +import sun.hotspot.WhiteBox; + import java.lang.ref.Cleaner; -import java.lang.ref.Reference; import java.lang.ref.PhantomReference; +import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; import java.lang.ref.SoftReference; import java.lang.ref.WeakReference; +import java.util.Arrays; import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Semaphore; @@ -34,19 +45,6 @@ import java.util.function.Consumer; import java.util.function.Supplier; -import jdk.internal.ref.PhantomCleanable; -import jdk.internal.ref.WeakCleanable; -import jdk.internal.ref.SoftCleanable; -import jdk.internal.ref.CleanerFactory; - -import sun.hotspot.WhiteBox; - -import jdk.test.lib.Utils; - -import org.testng.Assert; -import org.testng.TestNG; -import org.testng.annotations.Test; - /* * @test * @library /test/lib/share/classes /lib/testlibrary /test/lib @@ -85,16 +83,21 @@ void testCleanableActions() { Cleaner cleaner = Cleaner.create(); + // No actions + generateCases(cleaner); + // Individually - generateCases(cleaner, c -> c.clearRef()); - generateCases(cleaner, c -> c.doClean()); + generateCases(cleaner, CleanableCase::doClean); + generateCases(cleaner, CleanableCase::releaseReferent); // Pairs - generateCases(cleaner, c -> c.doClean(), c -> c.clearRef()); + generateCases(cleaner, + CleanableCase::doClean, + CleanableCase::releaseReferent); - CleanableCase s = setupPhantom(COMMON, cleaner); + CleanableCase s = setupPhantom(COMMON, cleaner).releaseReferent(); cleaner = null; - checkCleaned(s.getSemaphore(), true, "Cleaner was cleaned:"); + checkCleaned(s.getSemaphore(), 1, "Cleaner was cleaned:"); } /** @@ -112,24 +115,38 @@ void testRefSubtypes() { Cleaner cleaner = Cleaner.create(); + // No acions + generateCasesInternal(cleaner); + // Individually - generateCasesInternal(cleaner, c -> c.clearRef()); - generateCasesInternal(cleaner, c -> c.doClean()); - generateCasesInternal(cleaner, c -> c.doClear()); + generateCasesInternal(cleaner, CleanableCase::doClear); + generateCasesInternal(cleaner, CleanableCase::doClean); + generateCasesInternal(cleaner, CleanableCase::releaseReferent); // Pairs generateCasesInternal(cleaner, - c -> c.doClear(), c -> c.doClean()); + CleanableCase::doClear, + CleanableCase::doClean); + + generateCasesInternal(cleaner, + CleanableCase::doClear, + CleanableCase::releaseReferent); + + generateCasesInternal(cleaner, + CleanableCase::doClean, + CleanableCase::releaseReferent); // Triplets generateCasesInternal(cleaner, - c -> c.doClear(), c -> c.doClean(), c -> c.clearRef()); + CleanableCase::doClear, + CleanableCase::doClean, + CleanableCase::releaseReferent); generateExceptionCasesInternal(cleaner); - CleanableCase s = setupPhantom(COMMON, cleaner); + CleanableCase s = setupPhantom(COMMON, cleaner).releaseReferent(); cleaner = null; - checkCleaned(s.getSemaphore(), true, "Cleaner was cleaned:"); + checkCleaned(s.getSemaphore(), 1, "Cleaner was cleaned:"); } /** @@ -156,11 +173,11 @@ @SuppressWarnings("unchecked") void generateExceptionCasesInternal(Cleaner cleaner) { generateCases(() -> setupPhantomSubclassException(cleaner, null), - 1, c -> c.clearRef()); + 1, CleanableCase::releaseReferent); generateCases(() -> setupWeakSubclassException(cleaner, null), - 1, c -> c.clearRef()); + 1, CleanableCase::releaseReferent); generateCases(() -> setupSoftSubclassException(cleaner, null), - 1, c -> c.clearRef()); + 1, CleanableCase::releaseReferent); } /** @@ -174,7 +191,7 @@ @SuppressWarnings("unchecked") void generateCases(Supplier generator, int n, Consumer ... runnables) { - if (n == 1) { + if (n <= 1) { CleanableCase test = generator.get(); try { verifyGetRef(test); @@ -211,16 +228,26 @@ * @param test A CleanableCase containing the references */ void verify(CleanableCase test) { + + CleanableCase cc = setupPhantom(COMMON, test.getCleanable()).releaseReferent(); + // release Cleanable if impl. class + test.releaseCleanableIfImpl(); + System.out.println(test); - int r = test.expectedResult(); + int expectedCleanups = test.expectedCleanups(); - CleanableCase cc = setupPhantom(COMMON, test.getCleanable()); - test.clearCleanable(); // release this hard reference + checkCleaned(test.getSemaphore(), expectedCleanups, + test.cleanableDescr + " was cleaned:"); - checkCleaned(test.getSemaphore(), - r == CleanableCase.EV_CLEAN, - "Cleanable was cleaned:"); - checkCleaned(cc.getSemaphore(), true, + // release Cleanable unconditionally + test.releaseCleanable(); + + // only if the Cleanable is a CleanerImpl.XxxCleanableImpl class and + // it was not cleaned it will remain reachable... + int expectedCleanableCleanups = + (test.isCleanableImplClass && expectedCleanups == 0) ? 0 : 1; + + checkCleaned(cc.getSemaphore(), expectedCleanableCleanups, "The reference to the Cleanable was freed:"); } @@ -234,14 +261,16 @@ Reference r = (Reference) test.getCleanable(); try { Object o = r.get(); - Reference expectedRef = test.getRef(); - Assert.assertEquals(expectedRef.get(), o, - "Object reference incorrect"); - if (r.getClass().getName().endsWith("CleanableRef")) { + if (isCleanableImplClass(r.getClass())) { Assert.fail("should not be able to get referent"); } + Object expected = r instanceof PhantomReference + ? null + : test.getReferent(); + Assert.assertEquals(expected, o, + "Object reference incorrect"); } catch (UnsupportedOperationException uoe) { - if (r.getClass().getName().endsWith("CleanableRef")) { + if (isCleanableImplClass(r.getClass())) { // Expected exception } else { Assert.fail("Unexpected exception from subclassed cleanable: " + @@ -251,6 +280,15 @@ } /** + * @return true if given clazz is one of the CleanerImpl.XxxCleanableImpl + * classes + */ + static boolean isCleanableImplClass(Class clazz) { + return Cleaner.Cleanable.class.isAssignableFrom(clazz) && + clazz.getEnclosingClass() == CleanerImpl.class; + } + + /** * Test that releasing the reference to the Cleaner service allows it to be * be freed. */ @@ -274,35 +312,62 @@ } /** - * Check a semaphore having been released by cleanup handler. + * Check a semaphore having been released by cleanup handler for the number of times. * Force a number of GC cycles to give the GC a chance to process * the Reference and for the cleanup action to be run. * Use a larger number of cycles to wait for an expected cleaning to occur. * * @param semaphore a Semaphore - * @param expectCleaned true if cleaning should occur + * @param expectedCleanups # of expected cleanups to occur * @param msg a message to explain the error */ - static void checkCleaned(Semaphore semaphore, boolean expectCleaned, + static void checkCleaned(Semaphore semaphore, int expectedCleanups, String msg) { - long max_cycles = expectCleaned ? 10 : 3; + if (expectedCleanups < 0) { + // can't predict - anything is OK + whitebox.fullGC(); + int acquired = drain(semaphore); + System.out.printf(msg + " %d times\n", acquired); + return; + } + + long max_cycles = expectedCleanups > 0 ? 10 : 3; + // wait for at least 1 permit + int permits = Math.max(1, expectedCleanups); long cycle = 0; for (; cycle < max_cycles; cycle++) { // Force GC whitebox.fullGC(); try { - if (semaphore.tryAcquire(Utils.adjustTimeout(10L), TimeUnit.MILLISECONDS)) { - System.out.printf(" Cleanable cleaned in cycle: %d%n", cycle); - Assert.assertEquals(true, expectCleaned, msg); + if (semaphore.tryAcquire(permits, + Utils.adjustTimeout(10L), TimeUnit.MILLISECONDS)) { + int acquired = drain(semaphore) + permits; + System.out.printf(msg + " %d times in cycle %d\n", acquired, cycle); + Assert.assertEquals(acquired, expectedCleanups, msg); return; } } catch (InterruptedException ie) { // retry in outer loop } } - // Object has not been cleaned - Assert.assertEquals(false, expectCleaned, msg); + // cleanup has not been invoked for at least 'permits' # of times + int acquired = drain(semaphore); + System.out.printf(msg + " %d times\n", acquired); + Assert.assertEquals(acquired, expectedCleanups, msg); + } + + // like semaphore.drain() but wait a while for a permit to be available + private static int drain(Semaphore semaphore) { + int acquired = 0; + try { + while (semaphore.tryAcquire(Utils.adjustTimeout(10L), TimeUnit.MILLISECONDS)) { + acquired++; + } + } catch (InterruptedException e) { + // ignore + } + return acquired; } /** @@ -316,9 +381,9 @@ obj = new Object(); } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = cleaner.register(obj, () -> s1.release()); + Cleaner.Cleanable c1 = cleaner.register(obj, s1::release); - return new CleanableCase(new PhantomReference<>(obj, null), c1, s1); + return new CleanableCase(obj, c1, s1); } /** @@ -333,13 +398,13 @@ } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = new PhantomCleanable(obj, cleaner) { - protected void performCleanup() { + Cleaner.Cleanable c1 = new PhantomCleanable<>(obj, cleaner) { + public void clean() { s1.release(); } }; - return new CleanableCase(new PhantomReference<>(obj, null), c1, s1); + return new CleanableCase(obj, c1, s1); } /** * Create a CleanableCase for a WeakReference. @@ -353,13 +418,13 @@ } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = new WeakCleanable(obj, cleaner) { - protected void performCleanup() { + Cleaner.Cleanable c1 = new WeakCleanable<>(obj, cleaner) { + public void clean() { s1.release(); } }; - return new CleanableCase(new WeakReference<>(obj, null), c1, s1); + return new CleanableCase(obj, c1, s1); } /** @@ -374,13 +439,13 @@ } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = new SoftCleanable(obj, cleaner) { - protected void performCleanup() { + Cleaner.Cleanable c1 = new SoftCleanable<>(obj, cleaner) { + public void clean() { s1.release(); } }; - return new CleanableCase(new SoftReference<>(obj, null), c1, s1); + return new CleanableCase(obj, c1, s1); } /** @@ -395,14 +460,14 @@ } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = new PhantomCleanable(obj, cleaner) { - protected void performCleanup() { + Cleaner.Cleanable c1 = new PhantomCleanable<>(obj, cleaner) { + public void clean() { s1.release(); throw new RuntimeException("Exception thrown to cleaner thread"); } }; - return new CleanableCase(new PhantomReference<>(obj, null), c1, s1, true); + return new CleanableCase(obj, c1, s1, true); } /** @@ -417,14 +482,14 @@ } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = new WeakCleanable(obj, cleaner) { - protected void performCleanup() { + Cleaner.Cleanable c1 = new WeakCleanable<>(obj, cleaner) { + public void clean() { s1.release(); throw new RuntimeException("Exception thrown to cleaner thread"); } }; - return new CleanableCase(new WeakReference<>(obj, null), c1, s1, true); + return new CleanableCase(obj, c1, s1, true); } /** @@ -439,14 +504,14 @@ } Semaphore s1 = new Semaphore(0); - Cleaner.Cleanable c1 = new SoftCleanable(obj, cleaner) { - protected void performCleanup() { + Cleaner.Cleanable c1 = new SoftCleanable<>(obj, cleaner) { + public void clean() { s1.release(); throw new RuntimeException("Exception thrown to cleaner thread"); } }; - return new CleanableCase(new SoftReference<>(obj, null), c1, s1, true); + return new CleanableCase(obj, c1, s1, true); } /** @@ -460,57 +525,85 @@ */ static class CleanableCase { - private volatile Reference ref; - private volatile Cleaner.Cleanable cleanup; + private volatile Object referent; + private volatile Cleaner.Cleanable cleanable; private final Semaphore semaphore; private final boolean throwsEx; - private final int[] events; // Sequence of calls to clean, clear, etc. + final String cleanableDescr; + final boolean isCleanableImplClass; + private final Event[] events; // Sequence of calls to clean, clear, etc. private volatile int eventNdx; - public static int EV_UNKNOWN = 0; - public static int EV_CLEAR = 1; - public static int EV_CLEAN = 2; - public static int EV_UNREF = 3; - public static int EV_CLEAR_CLEANUP = 4; + enum Event { + UNKNOWN, CLEAR, CLEAN, RELEASE_REFERENT, RELEASE_CLEANABLE; + + boolean isBeforeIn(Event e, Event[] events) { + return this.indexIn(events) < e.indexIn(events); + } + + boolean isPresentIn(Event[] events) { + return this.indexIn(events) < events.length; + } + + int countOccurrencesIn(Event[] events) { + int count = 0; + for (int i = 0; i < events.length && events[i] != null; i++) { + if (events[i] == this) { + count++; + } + } + return count; + } + private int indexIn(Event[] events) { + for (int i = 0; i < events.length && events[i] != null; i++) { + if (events[i] == this) { + return i; + } + } + return events.length; + } + } - CleanableCase(Reference ref, Cleaner.Cleanable cleanup, + CleanableCase(Object referent, Cleaner.Cleanable cleanable, Semaphore semaphore) { - this.ref = ref; - this.cleanup = cleanup; - this.semaphore = semaphore; - this.throwsEx = false; - this.events = new int[4]; - this.eventNdx = 0; + this(referent, cleanable, semaphore, false); } - CleanableCase(Reference ref, Cleaner.Cleanable cleanup, + CleanableCase(Object referent, Cleaner.Cleanable cleanable, Semaphore semaphore, boolean throwsEx) { - this.ref = ref; - this.cleanup = cleanup; + this.referent = referent; + this.cleanable = cleanable; this.semaphore = semaphore; this.throwsEx = throwsEx; - this.events = new int[4]; + this.isCleanableImplClass = isCleanableImplClass(cleanable.getClass()); + this.cleanableDescr = cleanable.getClass().isAnonymousClass() + ? cleanable.getClass().getSuperclass().getName() + : cleanable.getClass().getName(); + this.events = new Event[5]; this.eventNdx = 0; } - public Reference getRef() { - return ref; + public Object getReferent() { + return referent; } - public void clearRef() { - addEvent(EV_UNREF); - ref.clear(); + public CleanableCase releaseReferent() { + if (referent != null) { + addEvent(Event.RELEASE_REFERENT); + referent = null; + } + return this; } public Cleaner.Cleanable getCleanable() { - return cleanup; + return cleanable; } public void doClean() { try { - addEvent(EV_CLEAN); - cleanup.clean(); + addEvent(Event.CLEAN); + cleanable.clean(); } catch (RuntimeException ex) { if (!throwsEx) { // unless it is known this case throws an exception, rethrow @@ -520,13 +613,25 @@ } public void doClear() { - addEvent(EV_CLEAR); - ((Reference)cleanup).clear(); + addEvent(Event.CLEAR); + ((Reference) cleanable).clear(); + } + + public void releaseCleanableIfImpl() { + if (isCleanableImplClass) { + // only the CleanerImpl.XxxCleanableImpl instances are guaranteed + // to be retained by CleanerImpl so we can only release them and + // still expect the Cleanable to be cleaned... + releaseCleanable(); + } } - public void clearCleanable() { - addEvent(EV_CLEAR_CLEANUP); - cleanup = null; + public void releaseCleanable() { + if (cleanable != null) { + // unconditionally release any Cleanable + addEvent(Event.RELEASE_CLEANABLE); + cleanable = null; + } } public Semaphore getSemaphore() { @@ -537,72 +642,61 @@ return semaphore.availablePermits() != 0; } - private synchronized void addEvent(int e) { + private synchronized void addEvent(Event e) { events[eventNdx++] = e; } /** - * Computed the expected result from the sequence of events. - * If EV_CLEAR appears before anything else, it is cleared. - * If EV_CLEAN appears before EV_UNREF, then it is cleaned. - * Anything else is Unknown. - * @return EV_CLEAR if the cleanup should occur; - * EV_CLEAN if the cleanup should occur; - * EV_UNKNOWN if it is unknown. + * Compute the number of expected cleanups to be triggered + * given the type of Cleanable implementation and the collected events. + * Return -1 if we can't predict the number. */ - public synchronized int expectedResult() { - // Test if EV_CLEAR appears before anything else - int clearNdx = indexOfEvent(EV_CLEAR); - int cleanNdx = indexOfEvent(EV_CLEAN); - int unrefNdx = indexOfEvent(EV_UNREF); - if (clearNdx < cleanNdx) { - return EV_CLEAR; - } - if (cleanNdx < clearNdx || cleanNdx < unrefNdx) { - return EV_CLEAN; - } - if (unrefNdx < eventNdx) { - return EV_CLEAN; - } + public synchronized int expectedCleanups() { - return EV_UNKNOWN; - } + if (isCleanableImplClass) { // the CleanerImpl.XxxCleanableImpl class - private synchronized int indexOfEvent(int e) { - for (int i = 0; i < eventNdx; i++) { - if (events[i] == e) { - return i; + // doesn't matter how many or what the order of events was as long + // as either the clean() method was called explicitly or + // the referent was released, we expect exactly one cleanup action + // otherwise none + return Event.CLEAN.isPresentIn(events) || + Event.RELEASE_REFERENT.isPresentIn(events) ? 1 : 0; + + } else { // plain XxxCleanable subclass + + // each time clean() is invoked explicitly, a cleanup action is + // performed (no at-most-once semantics here) + int explicitCleanups = Event.CLEAN.countOccurrencesIn(events); + + // if we clear()-ed the referent before releasing it, + // then that's all we get + if (Event.CLEAR.isBeforeIn(Event.RELEASE_REFERENT, events)) { + return explicitCleanups; } - } - return eventNdx; - } - private static final String[] names = - {"UNKNOWN", "EV_CLEAR", "EV_CLEAN", "EV_UNREF", "EV_CLEAR_CLEANUP"}; + // else only if Cleanable was not released and clear() was not + // called, can we expect any predictable behavior from plain + // XxxCleanable subclass... + if (!Event.RELEASE_CLEANABLE.isPresentIn(events) && + !Event.CLEAR.isPresentIn(events)) { + // we get one additional cleanup to all the explicit ones + // if referent was released... + return explicitCleanups + + (Event.RELEASE_REFERENT.isPresentIn(events)? 1 : 0); + } - public String eventName(int event) { - return names[event]; + // -1 means we can't predict + return -1; + } } public synchronized String eventsString() { - StringBuilder sb = new StringBuilder(); - sb.append('['); - for (int i = 0; i < eventNdx; i++) { - if (i > 0) { - sb.append(", "); - } - sb.append(eventName(events[i])); - } - sb.append(']'); - sb.append(", throwEx: "); - sb.append(throwsEx); - return sb.toString(); + return Arrays.asList(Arrays.copyOf(events, eventNdx)).toString(); } public String toString() { - return String.format("Case: %s, expect: %s, events: %s", - getRef().getClass().getName(), - eventName(expectedResult()), eventsString()); + return String.format("Case: %s, expected cleanups: %s, events: %s", + cleanableDescr, expectedCleanups(), eventsString()); } } @@ -617,11 +711,15 @@ String key = new String("foo"); // ensure it is not interned String data = "bar"; - map.put(new WeakKey<>(key, cleaner, map), data); + WeakKey k1 = new WeakKey<>(key, cleaner, map); + map.put(k1, data); WeakKey k2 = new WeakKey<>(key, cleaner, map); Assert.assertEquals(map.get(k2), data, "value should be found in the map"); + // ensure key is reachable until the lookup above + Reference.reachabilityFence(key); + // make key unreachable now key = null; System.gc(); Assert.assertNotEquals(map.get(k2), data, "value should not be found in the map"); @@ -634,28 +732,30 @@ } catch (InterruptedException ie) {} } Assert.assertEquals(map.size(), 0, "Expected map to be empty;"); - cleaner = null; + // ensure cleaner an k1 remain reachable until the end of the test + Reference.reachabilityFence(cleaner); + Reference.reachabilityFence(k1); } /** * Test sample class for WeakKeys in Map. * @param A WeakKey of type K */ - class WeakKey extends WeakReference { + class WeakKey extends WeakCleanable { private final int hash; private final ConcurrentHashMap, ?> map; - Cleaner.Cleanable cleanable; public WeakKey(K key, Cleaner c, ConcurrentHashMap, ?> map) { - super(key); + super(key, c); this.hash = key.hashCode(); this.map = map; - cleanable = new WeakCleanable(key, c) { - protected void performCleanup() { - map.remove(WeakKey.this); - } - }; } + + @Override + public void clean() { + map.remove(this); + } + public int hashCode() { return hash; } public boolean equals(Object obj) { @@ -664,13 +764,11 @@ } if (!(obj instanceof WeakKey)) return false; K key = get(); - if (key == null) return obj == this; - return key == ((WeakKey)obj).get(); + return key != null && key == ((WeakKey)obj).get(); } public String toString() { - return "WeakKey:" + Objects.toString(get() + ", cleanableRef: " + - ((Reference)cleanable).get()); + return "WeakKey:" + get(); } } @@ -702,9 +800,11 @@ // expected } + // ensure obj is reachable until now + Reference.reachabilityFence(obj); + // make it unreachable now obj = null; - checkCleaned(s1, true, "reference was cleaned:"); - cleaner = null; + checkCleaned(s1, 1, "reference was cleaned:"); } /** @@ -714,10 +814,8 @@ void testCleanerFactory() { Cleaner cleaner = CleanerFactory.cleaner(); - Object obj = new Object(); - CleanableCase s = setupPhantom(cleaner, obj); - obj = null; - checkCleaned(s.getSemaphore(), true, + CleanableCase s = setupPhantom(cleaner, null).releaseReferent(); + checkCleaned(s.getSemaphore(), 1, "Object was cleaned using CleanerFactor.cleaner():"); } }