7
\$\begingroup\$

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 <vector>
#include <mutex>
#include <algorithm>
#include <memory>

/**
 * Create the definition of a dispatcher for the event type (t)
 */
#define DISPATCHER(t) EventDispatcher<t>
/**
 * 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 <typename T>
class EventDispatcher {
public:

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

  class EventListener {
    friend EventDispatcher<T>;
  public:

    virtual ~EventListener() {
      std::unique_lock<std::recursive_mutex> 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<T> *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<T> *sender, std::shared_ptr<T> event) = 0;

  private:
    bool isAlive = true;
    typedef std::vector<EventDispatcher<T> *> DispatcherList;
    DispatcherList dispatchers;
    std::recursive_mutex lock;

    /**
     * Add a reference to the dispatchers this listener is attached to.
     * @param dispatcher The dispatcher that attached this listener.
     * @return true if the listener is still alive.
     */
    bool addDispatcher(EventDispatcher<T> *dispatcher) {
      if (dispatcher == NULL) {
        return false;
      }
      std::unique_lock<std::recursive_mutex> uLock(lock);
      if (isAlive) {
        if (std::find(dispatchers.begin(), dispatchers.end(), dispatcher) == dispatchers.end()) {
          // This should only ever be called by the dispatcher so no need to call addListener.
          dispatchers.push_back(dispatcher);
        }
      }
      return isAlive;
    }

    /**
     * Remove a reference to the dispatchers this listener is attached to.
     * @param dispatcher The dispatcher that removed this listener.
     */
    void removeDispatcher(EventDispatcher<T> *dispatcher) {
      if (dispatcher == NULL) {
        return;
      }
      std::unique_lock<std::recursive_mutex> uLock(lock);
      typename DispatcherList::iterator itr = std::find(dispatchers.begin(), dispatchers.end(), dispatcher);
      if (itr != dispatchers.end()) {
        // This should only ever be called by the dispatcher so no need to call removeListener.
        dispatchers.erase(itr);
      }
    }

  };

public:

  /**
   * Add a listener to the dispatcher.
   * @param listener The listener to add.
   */
  void addListener(EventDispatcher<T>::EventListener *listener) {
    if (listener == NULL) {
      return;
    }
    std::unique_lock<std::recursive_mutex> uLock(lock);
    if (isAlive) {
      if (std::find(listeners.begin(), listeners.end(), listener) == listeners.end()) {
        // Listener not in list, add it.
        if (listener->addDispatcher(this)) {
          // The listener was still alive so register it.
          listeners.push_back(listener);
        }
      }
    }
  }

  /**
   * Remove a listener from the dispatcher.
   * @param listener The listener to remove.
   */
  void removeListener(EventDispatcher<T>::EventListener *listener) {
    if (listener == NULL) {
      return;
    }
    std::unique_lock<std::recursive_mutex> uLock(lock);
    typename ListenerList::iterator itr = std::find(listeners.begin(), listeners.end(), listener);
    if (itr != listeners.end()) {
      listener->removeDispatcher(this);
      listeners.erase(itr);
    }
  }

protected:

  /**
   * Dispatch an event to all listeners.
   * @param event The event to dispatch.
   * @note If the event is modifiable then not all listeners will necessarily get the exact same message.
   * @note The event will be deleted before the function returns.
   */
  void dispatch(T *event) {
    std::shared_ptr<T> evt(event);
    std::unique_lock<std::recursive_mutex> uLock(lock);
    for (typename ListenerList::iterator iter = listeners.begin(); iter != listeners.end(); iter++) {
      (*iter)->onEvent(this, evt);
    }
  }

  /**
   * Dispatch an event to all listeners.
   * @param event The event to dispatch.
   * @note If the event is modifiable then not all listeners will necessarily get the exact same message.
   */
  void dispatch(std::shared_ptr<T> event) {
    std::unique_lock<std::recursive_mutex> uLock(lock);
    for (typename ListenerList::iterator iter = listeners.begin(); iter != listeners.end(); iter++) {
      (*iter)->onEvent(this, event);
    }
  }

private:
  bool isAlive = true;
  typedef std::vector<EventListener *> ListenerList;
  ListenerList listeners;
  std::recursive_mutex lock;

};

#endif  /* EVENT_H */

This is a simple test I've set up (not multi-threaded yet)

#include "include/Event.h"
#include <iostream>

class ActionEvent {
public:
  ActionEvent(int id) : actionID(id) {
//    actionID = id;
  }
  const int actionID;
};

class PropertyChangeEvent {
public:
  int propertyID;
};

class testDispatcher : public DISPATCHER(ActionEvent), public DISPATCHER(PropertyChangeEvent) {
public:
  void test() {
//    dispatch(new ActionEvent(0));  // ambiguous function.
//    dispatch(new PropertyChangeEvent());  // ambiguous function.
//    EventDispatcher<ActionEvent>::dispatch(new ActionEvent(1));  // works but is long.
//    EventDispatcher<ActionEvent>::dispatch(new ActionEvent(2));  // works but is long.
//    EventDispatcher<PropertyChangeEvent>::dispatch(new PropertyChangeEvent());  // works but is long.
    DISPATCH(ActionEvent, new ActionEvent(1));  // The dispatcher will make a shared pointer then delete the event.
    DISPATCH(ActionEvent, std::shared_ptr<ActionEvent>(new ActionEvent(2)));
    DISPATCH(PropertyChangeEvent, new PropertyChangeEvent());
  }
};

class testListener :
public LISTENER(ActionEvent), public LISTENER(PropertyChangeEvent) {
  int ID;
public:
  testListener(testDispatcher *dispatcher, int id) {
    ID = id;
//    dispatcher->addListener(this);  // ambiguous function.
//    dispatcher->addListener((EventDispatcher<ActionEvent>::EventListener *) this);  // ambiguous function.
//    ((EventDispatcher<ActionEvent> *)dispatcher)->addListener(this);  // works but is long.
    LISTEN(ActionEvent, dispatcher, this);
    if(id % 2) {
      // Only respond to property change events on odd numbered listeners just to be different.
//      dispatcher->addListener(this);  // ambiguous function.
//      ((EventDispatcher<PropertyChangeEvent> *)dispatcher)->addListener(this);  // works but is long.
      LISTEN(PropertyChangeEvent, dispatcher, this);
    }
  }

protected:
  void onEvent(EventDispatcher<ActionEvent> *source, std::shared_ptr<ActionEvent> event) {
    (void)source;
    (void)event;
    std::cout << ID << " ActionEvent " << event->actionID << std::endl;
//    event->actionID += 5;
//    std::cout << " set to " << event->actionID << std::endl;
  }

  void onEvent(EventDispatcher<PropertyChangeEvent> *source, std::shared_ptr<PropertyChangeEvent> event) {
    (void)source;
    (void)event;
    std::cout << ID << "PropertyChangeEvent" << std::endl;
  }
};

int main(int argc, char *argv[]) {
  testDispatcher td;
  testListener tl1(&td, 1);
  testListener tl2(&td, 2);
  testListener tl3(&td, 3);
  testListener tl4(&td, 4);
  td.test();

  return 0;
}

Is there any way I can improve it? Can I make the template easier to use (better macros)? Are there any possible gotchas I missed?

\$\endgroup\$
  • 2
    \$\begingroup\$ Please do not modify the original code after receiving answers. \$\endgroup\$ – Jamal Nov 9 '15 at 7:20
  • \$\begingroup\$ Apparently, It's standard procedure for moderators or whoever to remove additional information thats added to a question. Don't know why but it's making me wonder what the point of the sight is if you can't add anything new to the discussion... Anyway, if anyone wants to see my updated code just look in the edit history. \$\endgroup\$ – Robert Reinhart Nov 29 '15 at 1:18
  • \$\begingroup\$ It was removed because questions should only contain the original code or any code changes done before receiving a relevant answer. Updated code after answers can be posted in a separate answer, but it must have explanations behind the changes, otherwise it's prone to deletion. \$\endgroup\$ – Jamal Nov 29 '15 at 1:23
4
\$\begingroup\$

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<T>::dispatch(new T());
}

Use with

EventDispatcher<PropertyChangeEvent>::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<typename T, typename EDT>
void do_dispatch(EDT* disp, T* evt)
{
    disp->EventDispatcher<T>::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.

\$\endgroup\$
  • \$\begingroup\$ I posted some updated code. What do you think? Still need to deal with the listener possibly being deleted while an event is occurring. Have some Ideas on that but need time to try them. \$\endgroup\$ – Robert Reinhart Nov 28 '15 at 2:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.