[suggest] Listeners/Broadcasters unlisten in destructor


#1

As you know, I have my own homegrown generic listener/broadcaster system, which has been working well.

One improvement I have is that each listener keeps track of all the broadcasters that it’s listening to and removes itself from them on destruction (and of course vice versa).

The broadcasters have a lock, the listeners do not, and it seems to me that this means that I’m safe from both race conditions and deadlocks.

It also appears to me that add/removeListener are called infrequently compared to the actual callbacks, and so the marginal extra cost of the two-way system is more than worth the convenience, but more, the safety of knowing that all listeners are always removed automatically in every code path.

The reason why I’m writing this up is that one could add this to the broadcaster/listener system that’s already in Juce without changing the semantics at all. Well-written programs would already be removing the listeners in the destructors, so this new code would never actually remove anything.

I have to say that I did this with some trepidation, simply because Jules didn’t do this and I wondered if there were some subtle trap. But it worked really well - and it got rid of the very intermittent but annoying issue I had where I was calling back occasionally to a listener that had been deleted (later I realized it was a race condition in a destructor but I’d already avoided it with this global fix).


#2

I’ve thought about doing this, but it always felt slightly redundant to me.

My reasoning is: when you write a class that’s a listener, in 99% of cases, your object has some kind of pointer to the thing that it’s listening to - not surprising, because if it’s listening to it, then it’s interested in it, and objects generally have pointers to the things that they’re interested in. If you have an auto-deregistering listener, then the mechanism to do so needs to store its own pointer to the source object (or worse: an array of pointers to all the things it’s registered with), so your object ends up containing at least two pointers to the same thing. Of course in most cases this bit of duplicated data doesn’t matter, but if your objects are small and numerous, it could waste a large amount of space unnecessarily.

Something I have thought about doing is adding a debug mode sanity-checker that asserts if you try to delete an object while it’s still registered.


#3

My feeling on this is that it’s a space/reliability tradeoff - but for a tiny amount of space and a significant extra reliability boost.

Let’s suppose this adds 8 32-bit pointers on average per listener (my gut feeling is that it’s less than this as most are only listening to one thing) - so that’s 32 bytes extra per listener, about 50 millionths of a penny of RAM.

So that means 32,000 listeners would consume one extra meg of memory - just over a penny’s worth of memory.

In my experience, there are listeners attached to user interface things, or sources of data. I make heavy use of these - I might use three dozen listeners in my fairly large program. Let’s say I use 100 - that’s 3K.

Now consider the reliability issue. The problem that is being eliminated is that a listener is getting called after it’s been deleted. Now, as you know such problems can arise repeatably during development and fairly easily be debugged (but it’s still a few minutes of a programmer’s time) but they can also arise intermittently during production due to a race condition and cause a great deal of sorrow to some hardworking person.

Here’s a very reasonable scenario that should throw chills down your spine. During maintenance you change the order of two variables in a class. Now the listener is deleted just a little earlier than before. On your test systems, the threading model is such that the broadcaster can never see that deleted listener, but on some target systems in the field the timing is just a little different…

Reliability trumps memory. RAM is cheap, human time is not. We’re talking bytes, not even kilobytes!

And it’s less typing for the programmer, less to do, less to remember, one bug you can no longer make.


#4

Oh, I do agree that memory’s cheap these days, but it would just annoy me… In almost all the listeners that I write, it’s a simple case of addListener() in the constructor, removeListener() in the destructor, so really a complete no-brainer. The idea that a simple 4-byte class might be swollen to 5 or 6 times that size and have a pile of extra construction/destruction code, just to avoid a mistake that I know I haven’t made would rankle with me!

Perhaps a non-compulsory way to do it would be with an RAII class that you can embed in your object to manage the registering. It’d be pretty trivial to write, e.g.

[code]template <class ListenerType, class SourceObjectType>
class ScopedListener
{
public:
ScopedListener (SourceObjectType& source_)
: source (source_)
{
}

~ScopedListener()
{
    for (int i = listeners.size(); --i >= 0;)
        source.removeListener (listeners.getUnchecked(i));
}

void addListener (ListenerType* listener)
{
    listeners.add (listener);
    source.addListener (listener);
}

private:
SourceObjectType& source;
Array <ListenerType*> listeners;
};[/code]


#5

ScopedListener, great idea Jules and now it works with Button LOL


#6

ScopedListener, ok, but what happens if the Broadcaster is deleted before the listener (so we have a potential new problem) ?


#7

would be great to have a all-in-one solution which also respects the lifetime of objects ( therefore i use my DeletionSafePointer http://rawmaterialsoftware.com/viewtopic.php?f=2&t=6174&start=0 )


#8

Literally, since this thread started I had a tiny issue where I broke a class apart and the Juce-style “removeListener” didn’t make it across and I got an obscure failure in shutdown. The fact that it took me a minute to locate and fix because I’d spent time on this issue before didn’t make it a good thing!

chkn, you are quite right about “ScopedListener” - the only way to do it is to have the listener and broadcaster work together

“In almost all the listeners that I write, it’s a simple case of addListener() in the constructor, removeListener() in the destructor, so really a complete no-brainer.”

One could make the same argument against scoped pointers, eh? :smiley: It’s only a little work - but it’s work.

A connection to a broadcaster is a resource. Like most resources in C++, it’s best managed by Resource Acquisition Is Initialization.

You’ve seen my code - I write highly disciplined code, I like to think, but I write an awful lot of it and I want to write less and I want to be protected from having to spend a lot of time debugging a minor mistake like forgetting a destructor or in this case a removeListener. Other people are just mediocre programmers and should be protected from themselves to some appropriate extent between laiser-faire and a nanny state.

Knowing for a fact that there are no references left to my Listener once its destructor has completed, no matter what, makes me sleep better at night.

I write consumer code so my usage is perhaps different from yours but perhaps closer to a lot of your target audience.

For one thing, error handling makes up a surprisingly large amount of the body of the code - particularly around control structures like Listeners and Broadcasters. Even in my experiments on my own machine I’m constantly running into network dropouts, corrupt audio files, hitting cancel on dialogs, disks becoming unmounted and that sort of thing. As a result, I need to know that I can start to create any of my objects and then drop it on the floor at any point if there’s an error, without leaking any resources - like Broadcaster connections.

For another, since this is consumer software, there are a lot of small interface parts, so there are a large number of listeners and broadcasters, dozens of them, and I definitely hook them and unhook them at places that are not destructors.

Juce is one of the most RAII packages I know but I still found I had big destructors and issues in shutdown. When I went to enforcing my “Juce-derived objects must completely clean up after themselves” policy, my destructor for my biggest “ball of mud” class went from almost a page to two lines, and a nagging intermittent crash completely vanished. (That class is mostly gone now, thank Goodness, but these things are often unavoidable…)

APPENDIX: My generic listeners!

[code]#include

template class Broadcaster;

template
class Listener {
public:
typedef std::set<Broadcaster*> Broadcasters;
typedef typename Broadcasters::iterator iterator;

Listener() {}
virtual ~Listener();

virtual void operator()(Type x) = 0;

private:
Broadcasters broadcasters_;

friend class Broadcaster;
DISALLOW_COPY_AND_ASSIGN(Listener);
};

// Broadcast updates of type Type to a set of Listener.
template
class Broadcaster {
public:
typedef std::set<Listener*> Listeners;
typedef typename Listeners::iterator iterator;

Broadcaster() {}
virtual ~Broadcaster();

virtual void broadcast(Type x);
virtual void addListener(Listener* listener);
virtual void removeListener(Listener* listener);

protected:
CriticalSection lock_;
Listeners listeners_;

DISALLOW_COPY_AND_ASSIGN(Broadcaster);
};

template
Listener::~Listener() {
for (iterator i = broadcasters_.begin(); i != broadcasters_.end(); ++i)
(*i)->removeListener(this);
}

template
void Broadcaster::broadcast(Type x) {
ScopedLock l(lock_);
for (iterator i = listeners_.begin(); i != listeners_.end(); ++i)
(**i)(x);
}

template
Broadcaster::~Broadcaster() {
for (iterator i = listeners_.begin(); i != listeners_.end(); ++i)
(*i)->broadcasters_.erase(this);
}

template
void Broadcaster::addListener(Listener* listener) {
ScopedLock l(lock_);
listeners_.insert(listener);
listener->broadcasters_.insert(this);
}

template
void Broadcaster::removeListener(Listener* listener) {
ScopedLock l(lock_);
listeners_.erase(listener);
listener->broadcasters_.erase(this);
}

template
void add(Broadcaster* broadcaster, Listener* listener) {
broadcaster->addListener(listener);
}

template
void remove(Broadcaster* broadcaster, Listener* listener) {
broadcaster->removeListener(listener);
}
[/code]


#9

I like how Juce leaves it up to you to call addListener / removeListener, but at the same time you can bolt an add-on class into your code and get precisely the functionality you want.


#10

Actually, I think the best (and most RAII) solution to the whole thing would be to have three classes of object: broadcasters, listeners, and connections. If instead of calling add/removelistener, you created a “connection” object between a listener and broadcaster, then it’d let you do the job without every listener needing a list of all the things it’s registered with. The broadcaster would keep a list of connections, not listeners.

In fact, you could simulate something similar as a kind of “proxy listener” class, e.g.

[code]template <class ListenerClass, class BroadcasterClass>
class ListenerConnection : public ListenerClass
{
public:
ListenerConnection (ListenerClass& listener_, BroadcasterClass& broadcaster_)
: listener (listener_), broadcaster (broadcaster_)
{
broadcaster.addListener (this);
}

~ListenerConnection()
{
    broadcaster.removeListener (this);
}

void listenerCallback (some kind of params)
{
    listener.listenerCallback (params);
}

ListenerClass& listener;
BroadcasterClass& broadcaster;

};

class ActualListener : public SomeKindOfListener
{
public:
ActualListener()
{
connection = new ListenerConnection (*this, broadcaster);
}

ScopedPointer<ListenerConnection> connection;

};[/code]

…but obviously there are downsides, like having to write custom proxy classes for all the types of listener, and although the code is safer, it’s not as clear what’s going on, and would probably be fairly confusing for noobs.


#11

I actually experimented with a three-part system, but I dropped it.

There was nothing wrong with it at all - but I convinced myself it was identical to the two-part one in every way, there’s exactly the same information there and the same single lock held at the same time, the information is just arranged in separate objects rather than in one table.

The system I use (above) has just two classes, four methods and one lock (per broadcaster) and is two pages long. It’s short enough that I can say for sure that there is no combination of threading and calls to new, delete, addListener and removeListener that will possibly result in either a race condition or a dangling pointer.

I really like simple conditions on my data structures because there are just so many data structures. I don’t want to have to remember some trap like “not calling removeListener in the destructor”, because there are too many traps everywhere.

Looking at my code, I have over a dozen listeners, and in all but one case, I just declare the listener, and then call addListener for it - I lose one line of code for each listener.

When I went to the new scheme, I lost over a dozen calls to removeListener, but I also lost a half-a-dozen destructors to classes that only had them to get rid of listeners, and an entire class, which when the dust cleared was basically there to park listeners in.

But more to the point, I eliminated my removeListener errors. I have a rule that if I get bitten by the same bug twice, I take drastic steps to kill it forever. I got bitten once by simply forgetting to call removeListener, and then later a more subtle intermittent bug with a race condition and removeListener. The second time, I said, “Right!” and rolled up my sleeves.

With all due respect, I don’t buy that argument at all! The only extra freedom you get is to addListener and then NOT call removeListener when you are destroyed. This is exactly freedom to shoot yourself in the foot - you get no other ability to do things from it.

A system where Listeners self-remove from broadcasters on destruction doesn’t prevent you from doing anything at all. If for some reason you want to call removeListener yourself, that still works perfectly well.

Each Broadcaster already has to maintain a list of Listeners, so we’re less than doubling the RAM footprint of Listener/Broadcaster in the very worst case - and as I argued above, that footprint is tiny.

There’s only one lock in this system as I have it now - Broadcasters are locked but listeners aren’t.

I did this because under almost every conceivable pattern of code use that I can see you won’t be calling addListener for the same listener on different threads… but… now I write this I’m going to and put another lock there too (being careful to never keep both locks at the same time) because, again, I want this code to be correct and safe.


#12

It prevents you from using the memory required to remember the list of Broadcasters for other things, as well as the (admittedly small) number of CPU cycles required to add the Listener it’s list.

I still maintain that Juce got it right - the mandatory functionality is there, and if you want to be a mister fancy pants then you can bolt a RAII class onto it (with the associated costs).


#13

I still fail to see the advantage of having to manually unregister. What does it gain you? It seems to me the answer is “16 bytes per broadcaster/listener connection and nothing else”. I have a lot of listeners, but my estimate is that I’m spending about 1K of RAM to make sure they are automatically unlistened. We’re expecting people who are running our program to have at least 1 gig of RAM…

In particular, I feel that any argument you make against having self-unregistering listeners could be made against scoped pointers. There is never any time you want a reference to a listener after it’s been destroyed - there’s zero advantage of still having a pointer to that listener once the destructor has gone off.

Consider my motivating bug - I moved a “removeListener” call within the same class and then I started to get intermittent crashes.

I simply never want the possibility of such issues. I want my code to be stable under superficially correct changes, not have to debug it later and say, “Of course, it turns out that introduced a race condition!”

I don’t actually want to roll things myself if I can avoid it. I want things just to work and I want to have to learn as little as possible about them as I can in order to use them effectively and safely. There are only so many hours in the day to learn other people’s packages… my program unavoidably uses a half-a-dozen or so externals and dealing with their foibles is a fairly time-consuming job!

Your Mileage May Vary, of course. My theory is simply that I’ll outcompete you with more robust software and less client code. :wink:


#14

Tom, I’m completely on your side philosophically here, and will probably end up doing something like you’re suggesting.

My only niggle is that for some reason my most frequent scenario for using listeners seems to be where I have a parent component or object that listens for some events from its children or other embedded objects. In that case, since its children all get deleted before the parent, I never bother making my parent unregister itself. In that case, a more robust solution would add nothing but extra bulk and unnecessary work.


#15

99% of my Components have deleteAllChildren() in their destructor. Putting ScopedPointers in the class declaration only leads to me having to put the class declarations for my controls into header files, or forward declare them (which screws up Visual Studio 2008’s browse info). Most of my controls are self contained and I just write them as class declarations with all inline functions directly in the .cpp file containing the parent Component. Much easier that way.


#16
/*

 Program to look at various call-back solutions using C++ lambda 

 function objects.


 Typical Output: 


 Section time ********* std::function (ms): 4289.3

 Section time ********* hand-rolled function object (ms): 4459.3

 Section time ********* original juce solution (ms): 3010.7

 Program ended with exit code: 0


 */


#include "../JuceLibraryCode/JuceHeader.h"


/** A class that crudely measures how long between it's constructor

 and destruction.  Use to measure the time a function or block

 changes. */


#define jcfPerformanceTest(x) PerformanceTimer _perf_timer(x);


class PerformanceTimer {

public:

    PerformanceTimer(const String & _name) {

        name = _name;

        start = Time::getMillisecondCounterHiRes();

    }

    ~PerformanceTimer() {

        double now = Time::getMillisecondCounterHiRes();

        double delta = now - start;

        Logger::outputDebugString("Section time ********* " + name + " (ms): " + String(delta, 1));

    }

private:

    String name;

    double start;

};




class FunctionObjectHolderBase

{

public:

    virtual ~FunctionObjectHolderBase() {}

    virtual void execute() = 0;

};


template <class FunctionObjectType>

class FunctionObjectHolder

:

public FunctionObjectHolderBase

{

public:

    FunctionObjectHolder (const FunctionObjectType& f)

    :

    f (f)

    {}

    void execute()

    {

        f();

    }

private:

    FunctionObjectType f;

};


class XScopedActions;


class XScopedCallback

{

public:

    template <typename FunctionObject>

    XScopedCallback (const FunctionObject& callback)

    {

        storedAction = new FunctionObjectHolder<FunctionObject> (callback);

    }

    ~XScopedCallback();

    void execute()

    {

        storedAction->execute();

    }

    void removeCaller (XScopedActions* caller)

    {

        callers.removeFirstMatchingValue (caller);

    }

    void addCaller (XScopedActions* caller)

    {

        callers.addIfNotAlreadyThere (caller);

    }

private:

    ScopedPointer<FunctionObjectHolderBase> storedAction;

    Array<XScopedActions*> callers;

};


/** Use in a class to implement callbacks. */

class XScopedActions

{

public:

    ~XScopedActions()

    {

        for (auto& a : actions)

            a->removeCaller (this);

    }

    void call()

    {

        for (auto& a : actions)

            a->execute();

    }

    void add (XScopedCallback& s)

    {

        s.addCaller (this);

        actions.addIfNotAlreadyThere (&s);

    }

    void removeAction (XScopedCallback* s)

    {

        actions.removeFirstMatchingValue (s);

    }

private:

    Array<XScopedCallback*> actions;

};


inline XScopedCallback::~XScopedCallback()

{

    for (auto& c : callers)

        c->removeAction (this);

}


class ExampleButton {

public:

    void addButtonPushedCallback(XScopedCallback & callback)

    {

        buttonPushedCallback.add(callback);

    }

    void simulateButtonPush()

    {

        buttonPushedCallback.call();

    }

private:

    XScopedActions buttonPushedCallback;

};


//==============================================================================


template <class FuncType>

class ScopedActions;


class ScopedCallbackBase {

public:

    virtual ~ScopedCallbackBase() {}

};


typedef ScopedPointer<ScopedCallbackBase> ScopedCallbackPtr;


template <class FuncType>

class ScopedCallback

{

public:

    ScopedCallback (const FuncType & callback)

    {

        storedAction = callback;

    }

    ~ScopedCallback();

    void execute()

    {

        storedAction();

    }

    void removeCaller (ScopedActions<FuncType>* caller)

    {

        callers.removeFirstMatchingValue (caller);

    }

    void addCaller (ScopedActions<FuncType>* caller)

    {

        callers.addIfNotAlreadyThere (caller);

    }

private:

    FuncType storedAction;

    Array<ScopedActions<FuncType>*> callers;

};


/** Use in a class to implement callbacks. */

template <class FuncType>

class ScopedActions

{

public:

    ~ScopedActions()

    {

        for (auto& a : actions)

            a->removeCaller (this);

    }

    void call()

    {

        for (auto& a : actions)

            a->execute();

    }

    template <typename A1>

    void call(A1 & a1)

    {

        for (auto& a : actions)

            a->execute(a1);

    }

    template <typename A1, typename A2>

    void call(A1 & a1, A2 & a2)

    {

        for (auto& a : actions)

            a->execute(a1, a2);

    }

    template <typename A1, typename A2, typename A3>

    void call(A1 & a1, A2 & a2, A3 & a3)

    {

        for (auto& a : actions)

            a->execute(a1, a2, a3);

    }

    void add (ScopedCallback<FuncType>& s)

    {

        s.addCaller (this);

        actions.addIfNotAlreadyThere (&s);

    }

    /** A variation which you can submit either a function to and get a function

     pointer in return which can then be stored in a ScopedPointer<> */

    ScopedCallback<FuncType> * add(const std::function<FuncType> & function)

    {

        ScopedCallback<FuncType> * p = new ScopedCallback<FuncType>(function);

        p->addCaller(this);

        actions.add(p);

        return p;

    }

    

    void removeAction (ScopedCallback<FuncType>* s)

    {

        actions.removeFirstMatchingValue (s);

    }

private:

    Array<ScopedCallback<FuncType>*> actions;

};


template <class FuncType>

inline ScopedCallback<FuncType>::~ScopedCallback()

{

    for (auto& c : callers)

        c->removeAction (this);

}


typedef ScopedCallback<std::function<void(void)>>   VoidScopedCallback;

typedef ScopedActions<std::function<void(void)>>    VoidScopedActions;


//---

class TExampleButton {

public:

    void addButtonPushedCallback(VoidScopedCallback & callback)

    {

        buttonPushedCallback.add(callback);

    }

    void simulateButtonPush()

    {

        buttonPushedCallback.call();

    }

private:

    VoidScopedActions buttonPushedCallback;

};


//==============================================================================


class JuceExampleButton {

public:

    class Listener {

    public:

        virtual ~Listener() {}

        virtual void callback() = 0;

    };

    

    void addListener(Listener * l)

    {

        listeners.add(l);

    }

    void removeListener(Listener * l)

    {

        listeners.remove(l);

    }

    

    void simulateButtonPush()

    {

        listeners.call(&JuceExampleButton::Listener::callback);

    }

    

private:

    ListenerList<Listener> listeners;

};

                     

class JuceExampleCallbackObject :

public JuceExampleButton::Listener {

public:

    JuceExampleCallbackObject(int & j)

    :

    j(j)

    {}

    void callback() override {

        ++j;

    }

private:

    int & j;

};




//==============================================================================

int main (int argc, char* argv[])

{

    int j = 0;

    const int iterations = 20000;

    

    /* The solution wrapping in std::function. */

    {

        jcfPerformanceTest("std::function");

        

        for (int i = 0; i < iterations; ++i)

        {

            TExampleButton button;

            VoidScopedCallback printingCallback([&j]()

                                                {

                                                    ++j;

                                                });

            

            button.addButtonPushedCallback(printingCallback);

            button.simulateButtonPush();

            

            {

                VoidScopedCallback deletedCallback([&j]()

                                                   {

                                                       --j;

                                                   });

                button.addButtonPushedCallback(deletedCallback);

                for (int k = 0; k < iterations; ++k)

                    button.simulateButtonPush();

            }

            

            button.simulateButtonPush();

        }

    }


    {

        jcfPerformanceTest("hand-rolled function object");

        /* The solution using Function Objects. */

        for (int i = 0; i < iterations; ++i)

        {

            int j = 0;

            ExampleButton button;

            XScopedCallback printingCallback([&j]()

                                            {

                                                ++j;

                                            });

            

            button.addButtonPushedCallback(printingCallback);

            button.simulateButtonPush();

            

            {

                XScopedCallback deletedCallback([&j]()

                                               {

                                                   --j;

                                               });

                button.addButtonPushedCallback(deletedCallback);

                for (int k = 0; k < iterations; ++k)

                    button.simulateButtonPush();

            }

            

            button.simulateButtonPush();

        }

    }

    

    {

        jcfPerformanceTest("original juce solution");

        for (int i = 0; i < iterations; ++i)

        {

            int j = 0;

            JuceExampleButton button;

            JuceExampleCallbackObject callbackObject(j);

            

            button.addListener(&callbackObject);

            button.simulateButtonPush();

            

            {

                JuceExampleCallbackObject callbackObjectDeleted(j);

                button.addListener(&callbackObjectDeleted);

                for (int k = 0; k < iterations; ++k)

                    button.simulateButtonPush();

            }

            

            button.simulateButtonPush();

        }

    }

    

    return 0;

}

Sorry to dig up an old thread, but I had a poke at some other options for safe callbacks using lamda functions in C++11, and compared them with the standard Juce approach.

I think I'm going for the solution in TExampleButton.  A third slower is fine for me given the added convenience.  And the first template based idea I had wasn't exactly flexible - std::function was better :)

However I thought the results might be useful (and/or someone would tell me I'm doing something daft!).  Timings are at the top...