Event dispatcher / listener

I'm working on a program that will be significantly multi-threaded. I need to respond to events between objects and threads so I've come up with the following code.

```
#ifndef EVENT_H
#define EVENT_H

#include
#include
#include
#include

/**
* Create the definition of a dispatcher for the event type (t)
*/
#define DISPATCHER(t) EventDispatcher
/**
* Create the definition of a listener for event type (t)
*/
#define LISTENER(t) DISPATCHER(t)::EventListener
/**
* Dispatch a event (e) of type (t)
*/
#define DISPATCH(t, e) DISPATCHER(t)::dispatch(e);
/**
* Attach a listener (l) of type (t) to the dispatcher (d)
*/
#define LISTEN(t, d, l) ((DISPATCHER(t) *)(d))->addListener((l));

template
class EventDispatcher {
public:

virtual ~EventDispatcher() {
std::unique_lock uLock(lock);
// Prevent more listeners from being added.
isAlive = false;
// Remove all listeners.
while (listeners.begin() != listeners.end()) {
EventDispatcher::EventListener listener = listeners.begin();
// Call remove listener so that the listener will be notified of the change.
removeListener(listener);
}
}

class EventListener {
friend EventDispatcher;
public:

virtual ~EventListener() {
std::unique_lock uLock(lock);
// Stop more dispatchers from connecting.
isAlive = false;
// Remove self from all dispatchers.
// Use while loop as removeListener will call removeDispatcher and modify dispatchers.
while (dispatchers.begin() != dispatchers.end()) {
EventDispatcher dispatcher = dispatchers.begin();
dispatcher->removeListener(this);
}
}

protected:
/**
* Respond to an event.
* @param sender The dispatcher that sent the event.
* @param event The event that occurred.
*/
virtual void onEvent(EventDispatcher *sender, std::shared_ptr event) = 0;

private:
bool isAlive = true;
typedef std::vector *> DispatcherList;
DispatcherList di

Get rid of the macros. Now.

First of all too long is much better than totally breaks the type system and even makes really scary pointer casts. Second you can simply replace them with templates:

How about a new member for EventDispatcher:

void make_event() {
    EventDispatcher::dispatch(new T());
}

Use with

EventDispatcher::make_event();
// instead of 
//DISPATCH(PropertyChangeEvent, new PropertyChangeEvent());

Your homework is the perfect forwarding version of that (see make_unique). Or if you really insist on throwing around those pesky pointers, you can still make a friendly template

template
void do_dispatch(EDT* disp, T* evt)
{
    disp->EventDispatcher::dispatch(evt);
}

Use with

do_dispatch(this, new ActionEvent(1));
//DISPATCH(ActionEvent, new ActionEvent(1));

Rinse and repeat with the listener stuff. Remember: If you try to solve a problem with macros, you will have more problems afterwards.

Event lifetime

newing some Objects and then only later establishing a proper ownership via the shared_ptr is usually a recipe for subtle bugs that have to do with exceptions and order of argument evaluation. I presume you want to have the shared_ptrs there to go on with multithreading? Well then I can't really comment.

Threading

Well there's no threads in your code, so how could we possibly comment on that?

Listener lifetime

Actually the object lifetime of the Listener ends as soon as the destructor begins. So I have a really bad feeling about the fact that you only unregister the object while it technically already non-existent. I can't really pinpoint if there is actual undefined behavior but it feels dirty. No amount of locking and isAlive checks within the Listener can prevent that some thread is actually calling delete listener while another thread is executing it's methods. I instinctively want to curl up and crawl into a corner at the bare thought of that.

You should probably reconsider the ownership and lifetime of those such that you ensure that this can not happen. Also the fact that removeListener actually removes the dispatcher from the listener seems like a recipe for chaos. Maybe you want to look into std::weak_ptr which would allow you to keep non-owning pointers to for instance listeners, and if you want to notify them about an event, you can convert the weak_ptr into a shared_ptr and therefore extending the lifetime of the Listener at least until it is finished to process the event.

That said I don't see a specific point where your design is wrong. You seem to have put a lot of thought into it. But there are many invariants that you have to explicitly ensure. If you can make that less complex with more implicit management then its easier to avoid bugs.

StackExchange Code Review Q#109958, answer score: 4