A C#-style Event object in C++

Question

I wanted to make an Event object (similar to C# events) for use with a custom UI I'm writing. The Event should allow multiple consumers (observers) to attach to any given event and not leave dangling pointers if an observer disappears without explicitly detaching from it. It should also be thread safe, i.e. listeners should be able to attach or detach themselves from any thread. Finally, it should give me compilation errors if I try to dispatch an event with incorrect arguments, or attach to an event with an incorrect function signature.

My solution looks like this:

#include <tuple> #include <vector> #include <memory> #include <functional> #include <mutex> #include <iostream> #include <string> #include <cassert> #include <algorithm> class Non_Copyable { public: Non_Copyable() = default; Non_Copyable(Non_Copyable const &) = delete; Non_Copyable & operator = (Non_Copyable const &) = delete; }; template<typename T> class Event_Base : Non_Copyable { protected: using event_pair = std::pair<std::weak_ptr<void>, std::function<T>>; template<typename P, typename Q, typename... Args> void Attach_Internal(P(Q::*f)(Args...), std::shared_ptr<Q> const & p) { auto w = std::weak_ptr<Q>(p); assert(!Attached(w)); auto l = [w, f](Args... args) { if (auto locked = w.lock()) { return (*locked.get().*f)(args...); } else { return P(); } }; listeners.emplace_back(std::weak_ptr<void>(w), l); } void Detach_Internal(std::weak_ptr<void> const & p) { assert(Attached(p)); auto found = Find(p); if (found != listeners.end()) { listeners.erase(found); } } bool Attached(std::weak_ptr<void> const & p) { return Find(p) != listeners.end(); } void Clean() { listeners.erase(std::remove_if(std::begin(listeners), std::end(listeners), [&](event_pair const & p) -> bool { return p.first.expired(); }), std::end(listeners)); } typename std::vector<event_pair>::const_iterator Find(std::weak_ptr<void> const & p) const { if (auto listener = p.lock()) { return std::find_if(listeners.begin(), listeners.end(), [&listener](event_pair const & pair) { auto other = pair.first.lock(); return other && other == listener; }); } return listeners.end(); } std::vector<event_pair> listeners; }; template<typename T> class Event : public Event_Base<T> { public: template<typename P, typename Q, typename R, typename... Args> void Attach(P(Q::*f)(Args...), std::shared_ptr<R> const & p) { std::lock_guard<std::recursive_mutex> guard(mutex); this->Attach_Internal(f, p); } void Detach(std::weak_ptr<void> const & p) { std::lock_guard<std::recursive_mutex> guard(mutex); this->Detach_Internal(p); } template<typename... Args> void operator()(Args && ... args) { std::lock_guard<std::recursive_mutex> guard(mutex); if (!this->listeners.empty()) { this->Clean(); auto listeners_copy = this->listeners; for (auto const & listener : listeners_copy) { if (auto locked = listener.first.lock()) { listener.second(args...); } } } } int Count() const { return this->listeners.size(); } std::recursive_mutex mutex; }; 

Some questions...

Is my event object thread safe?

The public interface of Event is protected with a recursive_mutex. I chose recursive because it's quite possible that an event handler will detach the listener from the event its handling, or create objects that themselves will attach to that same event. This is also why I make a copy of the collection before dispatching the event.

Is my use of shared_ptr and weak_ptr correct?

The Event object maintains a vector of weak_ptr to listeners and I'm using the Clean() function to remove all disposed weak_ptr before dispatch of the event. This is a way for Event to know when to delete a listener from its collection (the listener weak_ptr will fail to lock).

Some things I don't like about it

Consumers cannot attach to events in their constructors because shared_from_this() is invalid in a constructor. For this reason Consumers need an Initialise.

std::recursive_mutex is considered harmful, though in this context I'm not sure that applies, does it?

It's probably quite slow, though as it's for UI events I don't expect any high frequency calling to happen.

Must use shared_ptr/weak_ptr in order to detect when listeners have dropped. I could I suppose make it part of the contract of use that listeners detach when they destroy themselves but... who reads the comments anyway?

Are there any improvements I could make to this code?

Here's some test code (append to above to use in cpp.sh or compile elsewhere).

class Producer { public: Event<void()> & On_Void_Event() { return on_void_event; } Event<void(int)> & On_Int_Event() { return on_int_event; } Event<void(int, int)> & On_Int_Int_Event() { return on_int_int_event; } Event<void(int, int, int)> & On_Int_Int_Int_Event() { return on_int_int_int_event; } void Fire_Events() { on_void_event(); on_int_event(1000); on_int_int_event(1000, 2000); on_int_int_int_event(1000, 2000, 3000); } private: Event<void()> on_void_event; Event<void(int)> on_int_event; Event<void(int, int)> on_int_int_event; Event<void(int, int, int)> on_int_int_int_event; }; class Consumer : public std::enable_shared_from_this<Consumer> { public: Consumer(int id) : id(id) {} void Initialise(Producer * producer) { assert(producer != nullptr); producer->On_Void_Event().Attach(&Consumer::Handle_Void_Event, shared_from_this()); producer->On_Int_Event().Attach(&Consumer::Handle_Int_Event, shared_from_this()); producer->On_Int_Int_Event().Attach(&Consumer::Handle_Int_Int_Event, shared_from_this()); producer->On_Int_Int_Int_Event().Attach(&Consumer::Handle_Int_Int_Int_Event, shared_from_this()); } void Handle_Void_Event() { std::cout << id << " Handling a void event" << std::endl; } void Handle_Int_Event(int value1) { std::cout << id << " Handling an int event (" << value1 << ")" << std::endl; } void Handle_Int_Int_Event(int value1, int value2) { std::cout << id << " Handling an int, int event (" << value1 << ", " << value2 << ")" << std::endl; } void Handle_Int_Int_Int_Event(int value1, int value2, int value3) { std::cout << id << " Handling an int, int, int event (" << value1 << "," << value2 << "," << value3 << ")" << std::endl; } private: int id; }; int main(void) { auto producer = std::make_shared<Producer>(); auto consumer1 = std::make_shared<Consumer>(1); auto consumer2 = std::make_shared<Consumer>(2); auto consumer3 = std::make_shared<Consumer>(3); auto consumer4 = std::make_shared<Consumer>(4); consumer1->Initialise(producer.get()); consumer2->Initialise(producer.get()); consumer3->Initialise(producer.get()); consumer4->Initialise(producer.get()); producer->Fire_Events(); return 0; } 

For completeness I added two other event types, Event_Functor and Event_Predicate. The former executes a function on the return result of each dispatch invocation , allowing the object firing the event to "gather" return results into an array, for example. The latter executes a predicate on the return result of each dispatch invocation, terminating dispatch if the predicate returns true.

I have a feeling using this in a observer pattern has a little of the code-smell about it but I imagine them being useful if you want to query your set of listeners for some return result.

template<typename T> class Event_Functor : public Event<T> { public: template<typename F, typename... Args> void operator()(F functor, Args && ... args) { std::lock_guard<std::recursive_mutex> guard(mutex); if (!this->listeners.empty()) { Clean(); auto listeners_copy = this->listeners; for (auto const & listener : listeners_copy) { if (auto locked = listener.first.lock()) { functor(listener.second(args...)); } } } } }; template<typename T> class Event_Predicate : public Event<T> { public: template<typename P, typename... Args> bool operator()(P p, Args && ... args) { std::lock_guard<std::recursive_mutex> guard(mutex); if (!listeners.empty()) { Clean(); auto listeners_copy = this->listeners; for (auto const & listener : listeners_copy) { if (auto locked = listener.first.lock()) { if (p(listener.second(args...))) { return true; } } } } return false; } }; 

Answer 1

Event is almost threadsafe

Why almost? Well, you don't take the lock in Count. That means that some thread could be making changes to your object while some other thread spontaneously decides to call Count, which will then be subject to a race condition. Even if you do read-only accesses, as long as the underlying object is not atomic, you have to lock.

Apart from that issue, Event seems to be threadsafe, at least as far as I can see.

Why std::recursive_mutex?

You do not do any recursive locking. There is absolutely no need for a std::recursive_mutex, as far as I can see. A normal std::mutex will do the job just fine.

To answer the question you posed: No, the fact that std::recursive_mutex is considered harmful does not apply here. However, you are not using any of the special functionality of it anyway, so why bother?

Other Things

1. About your inheritance scheme:

   class Event_Base : Non_Copyable { 

   Just don't. Don't do this. This is not the C++ way to do things, and can lead to all sorts of trouble down the road. For example, what about this:

   int main() { std::unique_ptr<Non_Copyable> p = std::make_unique<Event_Base>(); } 

   The behavior of that code is undefined, because Non_Copyable has no virtual destructor. And this is just one of the issues you are prone to run into when abusing inheritance, apart from your code becoming more and more inefficient.

2. What is the point of Event_Base? Do you plan to have other classes inherit from it? If yes, do add a virtual destructor, now! (See point 1)

   However, if I am honest, I really dislike the idea of having a base class as a kind of mixin or interface. The thing is this: Unless I am a child class of Event_Base, there is absolutely nothing I can do with an object of that class, since its whole interface is protected.

   Inheritance is not a zero cost abstraction in C++, and should be used sparingly, especially when performance is critical (which is not the case here, however). In your case, I would advocate for just joining the two classes Event_Base and Event into a single type. If you want to keep the Event_Base around, I could also imagine a different approach to tackling this issue: Convert Event_Base into a kind of event container (which it currently is not very far from) and replace instances of it as a superclass with member variables.

3. Event::operator() takes arguments by universal reference, but you don't do perfect forwarding. That means that you are locking yourself out of the benefits of move construction, which is usually the reason to use universal references in the first place. Whether this matters here at all is somewhat unclear to me and depends on your use case (do you ever expect a listener function moving from its arguments?). Still, it is weird to not see std::forward around there.

4. You can reduce the redundancy in the template parameter for Event while also improving type safety a little. What I mean is that the return type of the event functions you construct should always be void, since the result is discarded anyways. This enables you to actually reduce the template parameter from the full-blown function type to just the argument types, removing the need for writing out void(...) all the time and preventing the case in which somebody does not pay attention and tries to pass information through the return, which will inevitably end up in discarded-value-hell. As an added benefit, the lambda l in Attach_Inner would also become a lot simpler.