Is there a safe way to use Cleaner to unregister a listener?

Question

I have a Swing action class which works as follows:

package org.trypticon.hex.gui.datatransfer;

import java.awt.Toolkit;
import java.awt.datatransfer.Clipboard;
import java.awt.datatransfer.DataFlavor;
import java.awt.datatransfer.FlavorListener;
import java.awt.event.ActionEvent;
import javax.annotation.Nonnull;
import javax.swing.Action;
import javax.swing.JComponent;
import javax.swing.TransferHandler;

import org.trypticon.hex.gui.Resources;
import org.trypticon.hex.gui.util.FinalizeGuardian;
import org.trypticon.hex.gui.util.FocusedComponentAction;

public class PasteAction extends FocusedComponentAction {
    private final FlavorListener listener = (event) -> {
        // this method in the superclass calls back `shouldBeEnabled`
        updateEnabled();
    };

    @SuppressWarnings({"UnusedDeclaration"})
    private final Object finalizeGuardian = new FinalizeGuardian(() -> {
        Clipboard clipboard = Toolkit.getDefaultToolkit().getSystemClipboard();
        clipboard.removeFlavorListener(listener);
    });

    public PasteAction() {
        Clipboard clipboard = Toolkit.getDefaultToolkit().getSystemClipboard();
        clipboard.addFlavorListener(listener);
    }

    @Override
    protected boolean shouldBeEnabled(@Nonnull JComponent focusOwner) {
        TransferHandler transferHandler = focusOwner.getTransferHandler();
        if (transferHandler == null) {
            return false;
        }

        Clipboard clipboard = Toolkit.getDefaultToolkit().getSystemClipboard();
        DataFlavor[] flavorsInClipboard = clipboard.getAvailableDataFlavors();
        return transferHandler.canImport(focusOwner, flavorsInClipboard);
    }

    @Override
    protected void doAction(@Nonnull JComponent focusOwner) throws Exception {
        Action action = TransferHandler.getPasteAction();
        action.actionPerformed(new ActionEvent(
            focusOwner, ActionEvent.ACTION_PERFORMED, (String) action.getValue(Action.NAME)));
    }
}

The FinalizeGuardian referred to here is currently implemented using finalize():

package org.trypticon.hex.gui.util;

public final class FinalizeGuardian {
    private final Runnable cleanupLogic;

    public FinalizeGuardian(Runnable cleanupLogic) {
        this.cleanupLogic = cleanupLogic;
    }

    @Override
    protected final void finalize() throws Throwable {
        try {
            cleanupLogic.run();
        } finally {
            super.finalize();
        }
    }
}

So, for obvious reasons, I'd like to switch to using Cleaner for this.

The first try was something like this:

package org.trypticon.hex.gui.util;

import java.lang.ref.Cleaner;

public final class FinalizeGuardian {
    private static final Cleaner cleaner = Cleaner.create();

    public FinalizeGuardian(Runnable cleanupLogic) {
        cleaner.register(this, cleanupLogic);
    }
}

The problem is that now the object never becomes phantom reachable, because:

• Cleaner itself holds a strong reference to cleanupLogic
• cleanupLogic holds a reference to listener in order to remove the listener
• listener holds a reference to the action class in order to call updateEnabled on it
• the action class holds a reference to the FinalizeGuardian so that it doesn't get collected prematurely

Because the FinalizeGuardian itself never becomes phantom reachable, the cleaner will never be called.

So what I'd like to know is, is there a way to restructure this to follow the rules required to make Cleaner work correctly that doesn't involve breaking encapsulation by moving the listener to outside my action class?

Answer 1

As long as the FlavorListener is registered at an event source, it will never become unreachable (as long as the event source is still reachable). This implies that the PasteAction instance which the listener updates will also never become unreachable, as the listener has a strong reference to it.

The only way to decouple their reachability is to change the listener, to only maintain a weak reference to the object it updates. Note that when you’re using a Cleaner instead of finalize(), the FinalizeGuardian is obsolete.

The code would look like

public class PasteAction extends FocusedComponentAction {

    static FlavorListener createListener(WeakReference<PasteAction> r) {
        return event -> {
            PasteAction pa = r.get();
            if(pa != null) pa.updateEnabled();
        };
    }

    private static final Cleaner CLEANER = Cleaner.create();

    static void prepareCleanup(
                       Object referent, Clipboard clipboard, FlavorListener listener) {

        CLEANER.register(referent, () -> clipboard.removeFlavorListener(listener));
    }

    public PasteAction() {
        Clipboard clipboard = Toolkit.getDefaultToolkit().getSystemClipboard();
        FlavorListener listener = createListener(new WeakReference<>(this));
        clipboard.addFlavorListener(listener);
        prepareCleanup(this, clipboard, listener);
    }

…

Note that the critical parts have been placed into static methods, to make accidental capture of the this reference impossible. These methods get the minimum necessary to do their job, createListener only receives a weak reference to the action and prepareCleanup gets the referent as Object, as the cleaning action must not access any members of the action but receive the necessary values as separate parameters.

But after showing, how a cleaner use may look like, I have to strongly discourage from using this mechanism, especially as the only cleanup mechanism. Here, it is not only affecting memory consumption, but also the behavior of the program, because, as long as the references have not been cleared, the listener will keep getting informed and keep updating an obsolete object.

Since the garbage collector is only triggered by memory needs, it is perfectly possible that it doesn’t run or doesn’t care about these few objects, because there’s enough free memory, while the CPU is under heavy load, because of lots of obsolete listeners being busy to update obsolete objects (I’ve seen such scenarios in practice).

To make matters worse, with concurrent garbage collectors, it’s even possible that their collection cycle repeatedly overlaps with an actually obsolete execution of updateEnabled() triggered by the listener (because the reference was not cleared yet). Which will actively prevent the garbage collection of these objects, even when the garbage collector runs and would otherwise collect them.

In short, such cleanup should not rely on the garbage collector.