Event loop in C++

Question

Writing an EventLoop in C++. Few requirements, from a user perspective:

1. Must be simple to use (as simple as the Javascript event loop)
2. No type casting, no complex registering of event handler. Plain and simple API.
3. Event handlers have the proper event type passed to them as reference.
4. Should be able to register handlers and inject events in event handler themselves (prevent race conditions)
5. User can define events by just creating a class that derives from Event. There should be no need to implement a function or do anything in that class (POCO).
6. header only

From a maintainer perspective:

1. As static as possible (avoid dynamic cast, dynamic allocation, pointers...)
2. As efficient as possible (as few object copy as possible)

#include <algorithm>
#include <condition_variable>
#include <functional>
#include <iostream>
#include <map>
#include <mutex>
#include <queue>
#include <thread>
#include <typeindex>
// Pre-declaration
class Message;
class Event {};
// Represent a function that will be called with an event.
typedef std::function<void(Event&)> EventHandler;
// Utility function for our Event map.
template<typename T>
auto get_event_type() -> std::type_index {
 return std::type_index(typeid(T));
}
// User defined event. POCO!
class KeyDownEvent: public Event {
public:
 KeyDownEvent(int _key): key(_key) {}
 int key;
};
// Another user defined event.
class TimeoutEvent: public Event {
public:
 TimeoutEvent() {}
};
// A message is an event and its associated event handler. Designed inspired by
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/EventLoop.
class Message {
public:
 template <typename EventType>
 Message(EventType &&event, EventHandler event_handler) :
 m_event(std::make_unique<EventType>(std::move(event))),
 m_event_handler(event_handler) {}
 template <typename EventType>
 Message(Message&& message) :
 m_event(std::move(message.m_event)),
 m_event_handler(message.m_event_handler) {}
 std::unique_ptr<Event> m_event;
 EventHandler m_event_handler;
};
class EventLoop {
public:
 [[ noreturn ]] void run() {
 while (true) {
 std::unique_lock<std::mutex> lock(m_message_queue_mutex);
 // Wait for an event
 m_event_cv.wait(lock, [&]{ return m_message_queue.size() > 0; });
 // Retrieve the injected event
 Message message = std::move(m_message_queue.front());
 m_message_queue.pop();
 // Unlock before notify, is this necessary? Where did I saw that?
 lock.unlock();
 m_event_cv.notify_one();
 // Call the event listener
 message.m_event_handler(*message.m_event.get());
 }
 }
 template<class EventType>
 void add_event_listener(std::function<void(EventType&)> event_listener) {
 std::lock_guard<std::mutex> guard(m_event_listeners_mutex);
 std::type_index event_type = get_event_type<EventType>();
 // If the event_type as no listener yet...
 if (m_event_listeners.find(event_type) == m_event_listeners.end()) {
 // ... create the vector of listeners
 m_event_listeners[event_type] = {
 [&event_listener](Event& e) { event_listener(static_cast<EventType&>(e)); }
 };
 } else {
 // ... or append to the existing one.
 m_event_listeners[event_type].push_back([&event_listener](Event& e) {
 event_listener(static_cast<EventType&>(e));
 });
 }
 }
 // Convert an event to a message to be handled by the event loop.
 template <class EventType, typename ...EventParamsType>
 void inject_event(EventParamsType&&... event_params) {
 std::lock_guard<std::mutex> listener_guard(m_event_listeners_mutex);
 std::vector<EventHandler>& listeners = m_event_listeners[get_event_type<EventType>()];
 std::lock_guard<std::mutex> event_guard(m_message_queue_mutex);
 std::for_each(listeners.begin(), listeners.end(), [this, &event_params...](EventHandler &listener) {
 EventType event = EventType(std::forward<EventParamsType>(event_params)...);
 m_message_queue.emplace(std::move(event), listener);
 });
 m_event_cv.notify_one();
 }
private:
 std::queue<Message> m_message_queue;
 std::mutex m_message_queue_mutex;
 std::condition_variable m_event_cv;
 std::map<std::type_index, std::vector<EventHandler>> m_event_listeners;
 std::mutex m_event_listeners_mutex;
};

Could be used this way:

int main() {
 EventLoop eventLoop;
 std::thread mainThread(&EventLoop::run, &eventLoop);
 // Register event handlers
 eventLoop.add_event_listener<KeyDownEvent>([](KeyDownEvent& event) {
 std::cout << "*" << event.key << "*" << std::endl;
 });
 eventLoop.add_event_listener<TimeoutEvent>([](TimeoutEvent&) {
 std::cout << "timeout!" << std::endl;
 });
 do {
 int key = getchar();
 if (key != 10) {
 eventLoop.inject_event<KeyDownEvent>(key);
 }
 } while (true);
}

This EventLoop is basically an event Reactor.

• bipll: Why all those this->?
• Luke Skywalker: A bad habit I got from typescript which helps me separate local variable and class members.
• pacmaninbw: Welcome to code review. Fortunately or unfortunately this question might be considered off-topic because the example usage isn't even a function, let alone a class or code module.
• Luke Skywalker: @pacmaninbw, So if I put the example usage in a function would that make the question on-topic? Could you point me to the guidelines of the site?
• Malachi: Welcome to Code Review! Please see our Help center page for guidelines about posting new questions and getting the most from your post.

Answer 1

template <typename EventType>
Message(EventType &&event, EventHandler event_handler):
 m_event(std::make_unique<EventType>(std::move(event))),
 m_event_handler(event_handler) {}

Requiring an r-value event seems unnecessary here; we should probably take it by value.

---

 template <typename EventType>
 Message(Message&& message) :
 m_event(std::move(message.m_event)),
 m_event_handler(message.m_event_handler) {}

bug: This move constructor shouldn't have a template parameter.

---

 template<class EventType>
 void add_event_listener(std::function<void(EventType&)> event_listener) {
 ...
 if (m_event_listeners.find(event_type) == m_event_listeners.end()) {
 m_event_listeners[event_type] = {
 [event_listener](Event& e) { event_listener(static_cast<EventType&>(e)); }
 };
 }
 else {
 m_event_listeners[event_type].push_back([&event_listener] (Event& e) {
 event_listener(static_cast<EventType&>(e));
 });
 }
 }

bug: event_listener must be captured by value, not by reference, in both these lambdas (as a local variable it will be dead before the reference is used).

Note that we don't need the find and two separate branches; operator[] will add an empty vector for us so we can just do:

 m_event_listeners[event_type].push_back([event_listener] (Event& e) {
 event_listener(static_cast<EventType&>(e));
 });

---

[[noreturn]] void run() {
 while (true) {
 ...
 m_event_cv.notify_one();
 ...
 }
}

I don't understand the reason for calling m_event_cv.notify_one() here. Are we trying to allow multiple threads to call run()?

If so, they'll miss out on messages, since we've removed one from the queue.

If not, the next iteration of the run() loop will carry on processing if there are messages on the queue, since the predicate is checked before waiting. So there should be no need to call notify again.

---

template <class EventType, typename ...EventParamsType>
void inject_event(EventParamsType&&... event_params) {

The perfect forwarding here seems more complicated than passing in an EventType argument.

---

typedef std::function<void(Event&)> EventHandler;
...
Message(EventType event, EventHandler event_handler):
...
void add_event_listener(std::function<void(EventType&)> event_listener) {
...
 std::for_each(listeners.begin(), listeners.end(), [this, &event](EventHandler &listener) {

Consistency: we should choose either "listener" or "handler", and stick with it.

---

...
 std::for_each(listeners.begin(), listeners.end(), [this, &event_params...](EventHandler &listener) {
 EventType event = EventType(std::forward<EventParamsType>(event_params)...);
 m_message_queue.emplace(std::move(event), listener);
 });
...
std::queue<Message> m_message_queue;

We could perhaps:

• Make the Event argument to EventHandler a const&.
• Have an Event queue, instead of a Message queue (or technically a std::queue<std::pair<std::unique_ptr<Event>, std::type_index>> queue).
• Lock, copy, unlock, and call the relevant listeners for the event in the run() function.

This would allow us to avoid storing and copying the Event so many times. We'd also have one iteration of run() per event, instead of one iteration per listener per event.

Answer 2

Summary

Feel free to ignore opinion:

Personal opinion

I don't like "Snake Case" and I don't see it very often in C++ projects (though an argument against me is the C++ standard library does use it).

I also prefer not to use "m_" to identify member variables. It tends to mean that people have not though enough about unique meaningful names elsewhere and need to use the prefix to make things things unique. But on the other hand its not a negative (though I would advice adding build tools to enforce it long term so that half the code uses "m_" and another half the code fails to follow the convention.

I don't like the indention level is only two spaces. Its a bit small and makes it hard to read. I would prefer at least 4 spaces.

Why do half your functions use:

auto function() -> type

While half use the:

type function()

I would prefer to use one form consistently rather than a mixture. Though if you use the second form you can use the first form for those exceptional situations where it is required that you use the first form.

Code Review

Interesting that you even need an Event base if there are no virtual members.

class Event {};

---

The class EventLoop is a little dense in terms of code and comments and this makes it hard to read. This is definitely a case where the comments hinder the readability. You should simply remove these.

 [[ noreturn ]] void run() {
 while (true) {
 std::unique_lock<std::mutex> lock(m_message_queue_mutex);
 // Wait for an event
 m_event_cv.wait(lock, [&]{ return m_message_queue.size() > 0; });
 // Retrieve the injected event
 Message message = std::move(m_message_queue.front());
 m_message_queue.pop();
 // Unlock before notify, is this necessary? Where did I saw that?
 lock.unlock();
 m_event_cv.notify_one();
 // Call the event listener
 message.m_event_handler(*message.m_event.get());
 }
 }

Let's remove the comments and add some vertical white space and break into two logical functions. Note by using getNextEvent() I don't need to explicitly unlock() the lock as this is done as getNextEvent() exits with the destructor of lock.

 private:
 Message getNextEvent()
 {
 std::unique_lock<std::mutex> lock(m_message_queue_mutex);
 m_event_cv.wait(lock, [&]{ return m_message_queue.size() > 0; });
 Message message = std::move(m_message_queue.front());
 m_message_queue.pop();
 return message;
 }
 public:
 [[ noreturn ]] void run() {
 while (true) {
 Message message = getNextEvent();
 m_event_cv.notify_one();
 message.m_event_handler(*message.m_event.get());
 }
 }

---

Not sure a noreturn is appropriate.

 [[ noreturn ]] void run() {

Most application have a way to exit. So when the user selects exit your event loop should exit.

---

 // Unlock before notify, is this necessary? Where did I saw that?
 lock.unlock();
 m_event_cv.notify_one();

Is it necessary? No. It will still work either way.

Will it be more efficient? That will depend on the implementation. ITs hard to make a determination either way. But unlocking first will avoid a potential inefficiency. So I would do it this way but that does not provide anything.

---

You don't need an else here

 if (m_event_listeners.find(event_type) == m_event_listeners.end()) {
 m_event_listeners[event_type] = {
 [&event_listener](Event& e) { event_listener(static_cast<EventType&>(e)); }
 };
 } else {
 m_event_listeners[event_type].push_back([&event_listener](Event& e) {
 event_listener(static_cast<EventType&>(e));
 });
 }

I can simplify this too:

 m_event_listeners[event_type].push_back([&event_listener](Event& e) {
 event_listener(static_cast<EventType&>(e));
 });

This is because m_event_listeners has operator[]() will automatically insert an empty vector if that value does not exist.

---

To make this work you need to convert an Event object into the functions EventType so you use static_cast to achieve this:

[&event_listener](Event& e) { event_listener(static_cast<EventType&>(e)); }

This should be fine in "Simple" situations. But there are situations were "Simple" is not going to work. In these case's you are going to need dynamic_cast to make this work in all situations.

---

I don't see anything wrong inject_event() that I can complain about.

---

The one thing I normally see in event driven applications is that some handlers can swallow the event preventing subsequent handlers from performing actions based on the event.

Your code forces every event handler to handle the event. You could prevent this by adding a handled member to the Event base class. Then the lambda can check this value before calling the user provided haandler.

 class Event
 {
 bool handeled;
 public:
 virtual ~Event() {}
 Event(): handeled(false) {}
 bool isHandeled() const {return handeled;}
 };
 // .....
 m_event_listeners[event_type].push_back([&event_listener](Event& e) {
 try {
 if (!e.isHandeled()) {
 event_listener(dynamic_cast<EventType&>(e));
 }
 }
 catch(...) {
 // Event handlers are not written by me so I don't know how
 // they will work. I want to make sure exceptions in their
 // code don't cause the run() method to exit accidentally.
 // Do something to tell user there was an exception.
 }
 });

• Luke Skywalker: Thanks a lot for your review. Could you explain in what situation will the static_cast not work? Maybe with an example?
• Loki Astari: @LukeSkywalker no explicitly. But the rules for static_cast are very complex en.cppreference.com/w/cpp/language/static_cast and the results are usually UB which is hard to show that it is broken because UB can work on some compilers but not others.

Answer 3

Don't know if you dislike "not this-lang-onian" comments...

typedef std::function<void(Event&)> EventHandler;

should be

using EventHandler = std::function<void(Event&)>;

---

auto get_event_type() -> std::type_index {
 return std::type_index(typeid(T));
}

you already have type in return statement, no need for trailing return type

---

 Message(EventType &&event, EventHandler event_handler) :
 m_event(std::make_unique<EventType>(std::move(event))),

you already input event as r-value reference. no need for std::move

---

template <typename EventType>
Message(Message&& message) :
 m_event(std::move(message.m_event)),
 m_event_handler(message.m_event_handler) {}

1) doesn't depend on EventType, so doesn't need template wrapper
2) looks like standard move constructor for me, so

Message(Message&&) = default;

or even no mention of it would be enough

Also
If you do declare move constructor, by rule of five, you should say explicitly if you allow copy constructor, copy assignment and move assignment

---

void run() {
...
m_event_cv.notify_one();

I think I got the reason behind it - in case 2 events happens soon one after another (or, more probably, 2 messages from same event), the run() should loop over each of them...
But it's not done via notify_one Noone is listening for it to be notified!

Better approach probably would probably just copying queue, clearing the original and running for_all on local copy

lock.unlock(); here is critically important, as we don't want handlers that last long time to slow down event creators (that are waiting for m_message_queue_mutex)

---

message.m_event_handler(*message.m_event.get());

you're applying operator* to the inside raw pointer, while you can safely do that with unique_ptr too
(change *message.m_event.get() into *message.m_event)

---

So far everything looks good. All mistakes are minor. Logic structure is in place

Container for multiple derived classes in form of base class pointers is well known.
But this is first time I've ever seen container for functions that use derived class via wrapping function that takes base reference
Really clever

Thanks for experience! =)

• super: An rvalue-reference is actually an lvalue. So it most definitely needs to be moved if passed on. Have a look.
• Noone AtAll: @super TIL... i guess my intuition should've been "it has name - it is l-reference"