Do Java listeners need to be removed? (In general)

Question

Imagine this sample java class:

class A {
    void addListener(Listener obj);
    void removeListener(Listener obj);
}

class B {
    private A a;
    B() {
        a = new A();
        a.addListener(new Listener() {
            void listen() {}
        }
 }

Do I need to add a finalize method to B to call a.removeListener? Assume that the A instance will be shared with some other objects as well and will outlive the B instance.

I am worried that I might be creating a garbage collector problem here. What is the best practice?

Answer 1

When the B is garbage collected it should allow the A to be garbage collected as well, and therefore any references in A as well. You don't need to explicitly remove the references in A.

I don't know of any data on whether what you suggest would make the garbage collector run more efficiently, however, and when it's worth the bother, but I'd be interested in seeing it.

Answer 2

A will indeed keep B alive through the anonymous instance.

But I wouldn't override finalize to address that, rather use a static inner class who doesn't keep the B alive.

Answer 3

My understanding of the GC is that, until the removeListener method is called, class A will be maintaining a reference to the listener and so it won't be a candidate for GC cleanup (and hence finalize won't be called).

Answer 4

There is a cycle in the reference graph. A references B and B references A. The garbage collector will detect cycles and see when there are no external references to A and B, and will then collect both.

Attempting to use the finaliser here is wrong. If B is being destroyed, the reference to A is also being removed.

---

The statement: "Assume that the A instance will be shared with some other objects as well and will outlive the B instance." is wrong. The only way that will happen is if the listener is explicitly removed from somewhere other than a finalizer. If references to A are passed around, that will imply a reference to B, and B will not be garbage collected because there are external references to the A-B cycle.

---

Further update:

If you want to break the cycle and not require B to explicitly remove the listener, you can use a WeakReference. Something like this:

class A {
    void addListener(Listener obj);
    void removeListener(Listener obj);
}

class B {
    private static class InnerListener implements Listener {
        private WeakReference m_owner;
        private WeakReference m_source;

        InnerListener(B owner, A source) {
            m_owner = new WeakReference(owner);
            m_source = new WeakReference(source);
        }

        void listen() {
            // Handling reentrancy on this function left as an excercise.
            B b = (B)m_owner.get();
            if (b == null) {
                if (m_source != null) {
                    A a = (A) m_source.get();
                    if (a != null) {
                        a.removeListener(this);
                        m_source = null;
                    }
                }

                return;
            }
            ...
        }
    }

    private A a;

    B() {
        a = new A();
        a.addListener(new InnerListener(this, a));
    }
}

Could be further generalised if needed across multiple classes.

Answer 5

If you have added B as a listener to A, and A is meant to outlive B, the finalize call on B will never get called because there is an instance of B inside of A, so it will never get garbage collected. You could get around this by storing a reference to B in A as a WeakReference (which is not considered a reference during garage collection), but it would be better to explicitly deregister B from A when you no longer need it.

In general it is advised in Java to not use the finalize method in Java because you can never be sure when it will be called, and you can not use it to deregister yourself from another class.

Answer 6

You must be coming from C++ or some other language where people implement destructors. In Java you don't do that. You don't override finalize unless you really know what you're doing. In 10 years I never had to do that, and I still can't think of a good reason that would require me to do it.

Back to your question, your listener is an independent object with its own life cycle and will collected after all other objects that reference it will be collected or when no other object will be pointing to it. This works very well. So no, you don't have to override finalize.

Answer 7

A holds a reference to B through the anonymous instance in implicitly used by the anonymous type created. This means B won't be freed until removeListener is called, and thus B's finalize won't be called.

When A is destroyed, it's anonymous reference to B will also B destroyed opening the way to B being freed.

But since B holds a reference to A this never happens. This seems like a design issue - if A has a calls a listener, why do you need B to also hold a reference to A? Why not pass the A that made the call to the listener, if necessary?

Answer 8

A will indeed keep B from being garbage collected in you are using standard references to store your listeners. Alternatively when you are maintaining lists of listeners instead of defining new ArrayList<ListenerType>(); you could do something like new ArrayList<WeakReference<ListenerType>>();

By wrapping your object in a weakReference you can keep it from prolonging the life of the object.

This only works of course if you are writing the class that holds the listeners

Answer 9

How can A outlive B?:

Example Usage of B and A:

public static main(args) {
    B myB = new B();
    myB = null;
}

Behaviour I'd expect:

GC will remove myB and in the myB instance was to only reference to the A instance, so it will be removed too. With all their assigned listeners?

Did you maybe mean:

class B {
    private A a;
    B(A a) {
        this.a = a;
        a.addListener(new Listener() {
            void listen() {}
        }
}

With usage:

public static main(args) {
    A myA = new A();
    B myB = new B(myA);
    myB = null;
}

Because then I would really wonder what happens to that anonymous class....

Answer 10

In your situation the only garbage collection "problem" is that instances of B won't be garbage collected while there are hard-references to the shared instance of A. This is how garbage collection supposed to work in Java/.NET. Now, if you don't like the fact that instances of B aren't garbage-collected earlier, you need to ask yourself at what point you want them to stop listening to events from A? Once you have the answer, you'll know how to fix the design.

Answer 11

Building on what @Alexander said about removing yourself as a listener:

Unless there is some compelling reason not to, one thing I've learned from my co-workers is that instead of making an anonymous inner Listener, and needing to store it in a variable, make B implement Listener, and then B can remove itself when it needs to with a.removeListener(this)

Answer 12

I just found a huge memory leak, so I am going to call the code that created the leak to be wrong and my fix that does not leak as right.

Here is the old code: (This is a common pattern I have seen all over)

class Singleton {
    static Singleton getInstance() {...}
    void addListener(Listener listener) {...}
    void removeListener(Listener listener) {...}
}

class Leaky {
    Leaky() {
        // If the singleton changes the widget we need to know so register a listener
        Singleton singleton = Singleton.getInstance();
        singleton.addListener(new Listener() {
            void handleEvent() {
                doSomething();
            }
        });
    }
    void doSomething() {...}
}

// Elsewhere
while (1) {
    Leaky leaky = new Leaky();
    // ... do stuff
    // leaky falls out of scope
}

Clearly, this is bad. Many Leaky's are being created and never get garbage collected because the listeners keep them alive.

Here was my alternative that fixed my memory leak. This works because I only care about the event listener while the object exists. The listener should not keep the object alive.

class Singleton {
    static Singleton getInstance() {...}
    void addListener(Listener listener) {...}
    void removeListener(Listener listener) {...}
}

class NotLeaky {
    private NotLeakyListener listener;
    NotLeaky() {
        // If the singleton changes the widget we need to know so register a listener
        Singleton singleton = Singleton.getInstance();
        listener = new NotLeakyListener(this, singleton);
        singleton.addListener(listener);
    }
    void doSomething() {...}
    protected void finalize() {
        try {
            if (listener != null)
                listener.dispose();
        } finally {
            super.finalize();
        }
    }

    private static class NotLeakyListener implements Listener {
        private WeakReference<NotLeaky> ownerRef;
        private Singleton eventer;
        NotLeakyListener(NotLeaky owner, Singleton e) {
            ownerRef = new WeakReference<NotLeaky>(owner);
            eventer = e;
        }

        void dispose() {
            if (eventer != null) {
                eventer.removeListener(this);
                eventer = null;
            }
        }

        void handleEvent() {
            NotLeaky owner = ownerRef.get();
            if (owner == null) {
                dispose();
            } else {
                owner.doSomething();
            }
        }
    }
}

// Elsewhere
while (1) {
    NotLeaky notleaky = new NotLeaky();
    // ... do stuff
    // notleaky falls out of scope
}