EventBus in C++

Question

Edit

I do know the example key event doesn't carry any data (like keycode) but that isn't really the point of the review - the overall setup is :)

Original

After reading quite few suggestions on this StackExchange and doing some additional research, I have decided to give it a try and implement my own Event system.

The use case for the system is to use it in a small game engine further down the line. I have decided to go for a subscriber model with a singleton EventBus.

Idea behind implementation My goal was to have a system which can scale but doesn't require much overhead by the developer. The shortcoming may be having to handle all events in one handle method - but that is something I am fine with as you can easily filter on event types.

For storing I have decided to use a linked list. I have been considering using maps, hashmaps, or arrays, but in the end decided to go for linked list as I feel like it gives me the most control over the data and cleanup.

The events are stored in a queue and processed in a single process_queue call as I expect that to happen once every frame. There is also an option to stop propagation by returning true from handle callback (yes, I am a web developer :) )

Finally I would like your opinion on this implementation and also if you can see any shortcomings I may have missed.

Thank you ever so much! Code below...

Macros and types

#define BIT(x) 1 << x
template<typename T>
struct ListNode {
 T* value;
 struct ListNode<T>* next;
};
enum EventType {
 None = 0,
 KeyPressed = BIT(0), KeyReleased = BIT(1)
};
struct Event {
 explicit Event(EventType type) : m_type(type) { }
 [[nodiscard]] inline EventType get_type() const {
 return m_type;
 }
 [[nodiscard]] inline bool is_type(EventType type) const {
 return m_type == type;
 }
 private:
 EventType m_type;
};

Interfaces

struct IEventHandler;
struct IEventBus {
 public:
 virtual void push_to_queue(Event&& event) = 0;
 virtual void process_queue() = 0;
 private:
 virtual ListNode<IEventHandler>* register_handler(IEventHandler* handler) = 0;
 virtual void unregister_handler(ListNode<IEventHandler>* node) = 0;
};
struct IEventHandler {
 virtual bool handle(const Event& event) = 0;
 virtual inline int get_signature() const = 0;
};

Implementation

struct EventBus : public IEventBus {
 public:
 void push_to_queue(Event&& event) override {
 m_queue.emplace(event);
 }
 void process_queue() override {
 if (m_head == nullptr) {
 return;
 }
 while (!m_queue.empty()) {
 auto event = m_queue.front();
 auto tail = m_head;
 while (tail != nullptr) {
 auto handler = tail->value;
 if (handler->get_signature() & event.get_type()) {
 auto stop_propagation = handler->handle(event);
 if (stop_propagation) {
 break;
 }
 }
 // move to next handler
 tail = tail->next;
 }
 m_queue.pop();
 }
 }
 static EventBus& get_instance() {
 static EventBus instance;
 return instance;
 }
 EventBus(EventBus const&) = delete;
 void operator=(EventBus const&) = delete;
 ListNode<IEventHandler>* register_handler(IEventHandler* eventHandler) override {
 auto node = new ListNode<IEventHandler> { eventHandler, nullptr };
 // check if handler is new head
 if (m_head == nullptr) {
 m_head = node;
 } else {
 auto tail = m_head;
 while (tail->next != nullptr) {
 tail = tail->next;
 }
 tail->next = node;
 }
 // return node
 return node;
 }
 void unregister_handler(ListNode<IEventHandler>* node) override {
 assert(m_head != nullptr && "Something went wrong - the handler list is null");
 // find parent of node
 auto tail = m_head;
 while (tail->next != node) {
 tail = tail->next;
 }
 // now we have parent - yay! change the reference to connect the list
 tail->next = node->next;
 // the listnode is not "detached" and can be removed manually. the data is destroyed (as the IEventHandler unregisters in destructor)
 delete node;
 }
 private:
 EventBus() = default;
 private:
 ListNode<IEventHandler>* m_head { nullptr };
 std::queue<Event> m_queue {};
};
struct EventHandler : public IEventHandler {
 explicit EventHandler(int handlerSignature): m_handlerSignature(handlerSignature) {
 m_node = EventBus::get_instance().register_handler(this);
 }
 ~EventHandler() {
 EventBus::get_instance().unregister_handler(m_node);
 }
 [[nodiscard]] inline int get_signature() const override {
 return m_handlerSignature;
 }
 private:
 ListNode<IEventHandler>* m_node;
 int m_handlerSignature;
};

Use-case example

struct KeyPressEvent : public Event {
 KeyPressEvent() : Event(EventType::KeyPressed) { }
};
struct Actor : public EventHandler {
 Actor(): EventHandler(EventType::KeyPressed | EventType::KeyReleased) { }
 bool handle(const Event& event) final {
 // only events of type EventType::KeyPressed/EventType::KeyReleased will be handled here
 if (event.is_type(EventType::KeyPressed)) {
 std::cout << "Hey! You pressed a key!\n";
 }
 return true;
 }
};
int main() {
 // Create event
 KeyPressEvent event;
 // Create actor(s)
 auto actor = new Actor();
 auto actor2 = new Actor();
 auto actor3 = new Actor();
 // Dispatch event
 EventBus::get_instance().push_to_queue(std::move(event));
 // Process queue
 EventBus::get_instance().process_queue();
 return 0;
}
```

• G. Sliepen: So now you know a key is pressed, but which one? And if you add a MouseMovement event, how do you pass the mouse coordinates?
• Jiri Kralovec: You can set whatever data you need in the Event subclasses, and later differentiate based on EventType when handling the event. The example really is limited, real case would 100% include more diverse events types/classes.
• Alex: Do you expect to often register/unregister event handlers?
• Keiji: I've only skimmed your question and code snippets - but I'm wondering if you've heard of Pony (ponylang.io). That language seems to be made exactly for your use case (handling multiple actors passing events around), and if this is a new project you're just starting, and you've got no particular reason to use C++ specifically, and you're happy to use a new, not-1.0-yet language, it might be exactly what you're looking for.
• Jiri Kralovec: @Keiji I do want to use C++, also partly because I want to keep learning it, but I will give Pony a look. It may also give me ideas on how to approach things differently - thanks for the tip!

Answer 1

 EventBus::get_instance().push_to_queue(std::move(event));
 EventBus::get_instance().process_queue();

Those seem inconvenient for the caller to write. Recommend you offer a singleton-aware facade, so caller only needs to know about push_to_queue and process_queue convenience functions.

---

You mentioned that you want to use this in a 60 FPS game use case. So let's talk about performance. There's no timing data in the submission, limiting us to just guesses and hand waving.

App developers will want to know how much headroom they have, how many more tasks they could safely enqueue without falling behind. Consider maintaining a max_queue_length variable that developers can keep an eye on when they add new event sources.

There's no timestamps in the engine, and that's fine. Publish an example "tick" handler: we enqueue current clock, and then the handler reports the delta time.

Most folks think of Linked List as a "slow" datastructure, since there's only sequential access and the CPU is waiting on random read for each next dereference. In contrast an Array offers cache-friendly sequential reads that cooperate with hardware prediction and prefetch.

An advantage is we can always allocate and link in One More node. But honestly, I am skeptical that you really want that. If enqueues happen significantly faster than events are handled, you really want to signal fatal error to the caller, rather than consume all of memory with backlogged events.

Consider using a (circular) array, instead. The head and tail indexes will cycle through the array, chasing one another. Do a realloc on the array if handlers get moderately behind. You might choose to fail the enqueue request if there's no room and the array has already been resized to be "big".

---

Most deployment targets these days have more than one core. Enqueuing events can be a very convenient approach to scheduling work.

There is apparently a single thread (on a single core) that runs handlers. You will probably want a bigger scheduling story before long.

Ignoring the current Singleton focus for a moment, you could deploy K instances of this code on K cores. And then when we're enqueuing we're burdened with choosing appropriate instance.

We could use the current Singleton API, and behind the scenes a target thread is chosen at enqueue time.

Or we could use the current code, with the decision to schedule on one of K threads deferred until the last moment.

Now let's talk about the implicit Happens Before relationships induced by the current code. After enqueuing A, B, then C, an app developer might reasonably expect that B only begins after A finishes, and similarly for C. If you want to guarantee that, fine. But you might want to reserve greater scheduling latitude, so that future releases of this engine scale up to effectively use more cores. Spell out the rules now, when no app depends on specific behavior yet.

Suppose an app developer doesn't care when B runs as long as it eventually does, but he needs A to finish before C begins. You may need to offer a way to express that. No need to tackle it now; wait for a motivating use case to crop up.

Suppose that A & C both manipulate the same lock, and additionally they interact with some more locks. Developers will look to you to offer advice on how to avoid deadlock in your engine.

Answer 2

std::forward_list

Is there a specific reason you are re-implementing a linked list inside of your event bus? Why not use std::forward_list (or std::list for doubly-linked)? If you wanted to re-implement it yourself to learn, that's great! But then I would suggest to implement it on the side and use the list inside of your class.

EDIT: looking at another response, unless you have an iterator to the previous element in your std::forward_list, you cannot delete in O(1), so you should use a std::list instead.

std::forward_list vs std::vector performance

Going one step further, as this is an environment that might require some performance depending on the kind of game you will implement, you could then template your event bus on the type of container it uses, and easily benchmark your system to see if using a linked list is beneficial in your case (I know that you can delete in the middle of the list in O(1) in a list and not in a vector, but in practice as a vector is able to leverage your cpu caches, it is sometimes still better to use a std::vector) (note: This is where my question Do you expect to often register/unregister event handlers? comes in, because if you want to benchmark your event system you need to feed input that is representative of its usage).

Separating events into several lists

Another high-level question, depending on the number of events you have, you might want to not keep all your event handlers in one list. For example, in a shooting game, let's say you have a keyboard input event, and a headshot event. 99% of your events will be input events and 1% will be the head shots, but for every input event you will load the head shot event handler from memory and check if its flag matches, and just do nothing. Instead, you could separate your event handlers into several lists, one for each event type (which is easier if you extract your data structure from your class). This way, when you get an input event, you just call all your handlers from your input handlers list, and your head shot event handler is not even loaded from memory anymore. There is a drawback to this solution: if you regularly create and delete event handlers which handle several kinds of events, you will need to add and remove them from all the lists every time (so it also depends on your use case).

Implementation

From a birds eye view, you've done a really good job. Your code is clean, you keep the same coding style throughout, and it is easy to understand, and the architecture looks really nice to me (besides the integration of the linked list inside of the event bus).

Macros

#define BIT(x) 1 << x, I would use macros as little as possible and replace this by a constexpr function:

constexpr std::size_t bit(std::size_t n) {
 return 1 << n;
}

process_queue

For the process_queue method, is it expected behaviour that your events accumulate if there is no handler to treat them? Because if there is a handler in the list, and that handler does not handle the event, the event still gets removed.

Methods defined inside the class definition

[[nodiscard]] inline EventType get_type() const {
 return m_type;
}
[[nodiscard]] inline bool is_type(EventType type) const {
 return m_type == type;
}

Really good to include nodiscard there, inline is optional as the method is defined in the class.

Smart pointers

auto node = new ListNode<IEventHandler> { eventHandler, nullptr };

You should not use raw pointers and new here, instead because those are owning pointers you should use std::unique_ptr to implement your linked list (note: if you have a lot of elements in your list, you need to be careful when destroying the std::unique_ptr list, see this post). I think there could even be a memory leak here (not 100% sure though, which is one reason to use smart pointers).

Bit manipulations

virtual inline int get_signature() const = 0;

It is generally recommended to use unsigned integer types when using bitwise operators.

Copy assignment operator signature

void operator=(EventBus const&) = delete;

operator= should return EventBus&.

• Davislor: This is great advice, for an implementation that doesn’t need to be thread-safe! Lock-free algorithms would typically use a linked list.

Answer 3

Use std::list instead of writing your own list implementation

You are already using the standard library for some containers, so it is weird to see that you decided to write your own list implementation instead of just using std::list. And if you really want a singly-linked list, you can use std::forward_list, although the latter doesn't allow \$O(1)\$ removal of a given element.

struct EventBus : public IEventBus {
 std::list<IEventHandler*> m_handlers;
 using Registration = decltype(m_handlers)::iterator;
 ...
 Registration register_handler(IEventHandler* eventHandler) override {
 return m_handlers.insert(m_handlers.end(), eventHandler);
 }
 void unregister_handler(Registration registration) override {
 m_handlers.erase(registration);
 }
 ...
};

The event queue doesn't handle derived classes

You mentioned that the intention is that classes derive from Event, and store any data related to the derived event type as member variables in the derived class. However, you store events in a std::queue<Event>. Because you store Events by value, any member variables stored in derived classes are lost (this is called object slicing).

The correct way to store objects of potentially derived classes is to use pointers. To avoid manual memory management, you can have the queue store std::unique_ptrs:

std::queue<std::unique_ptr<Event>> m_queue;

Note that this also requires that you add a virtual destructor to Event, otherwise the destructor of the derived class is never called when removing elements from m_queue.

Don't make it a singleton

Maybe you only need one EventBus in the application you have in mind, but that is not a good reason to make the class a singleton. Consider that in C++, you can just create a global variable, and use that instead of calling EventBus::get_instance() everywhere:

EventBus the_event_bus;
int main() {
 KeyPressEvent event;
 ...
 the_event_bus.push_to_queue(std::move(event));
 the_event_bus.process_queue();
}

In the above example you could even make the_event_bus a local variable inside main() of course.

There is no need for IEventBus

While you need a base class for the event handlers and the events in your code, you don't need a base class for EventBus, so I would just remove it.

Avoid macros

Your macro BIT() is unsafe. What if you write BIT(2 & 2)? The result will be 0, but you expected 4. What if you write BIT(2) + 2? The result will be 16, but you probably expected 6. You could fix the macro by using parentheses:

#define BIT(x) (1 << (x))

But even better is to make it a constexpr function:

static constexpr unsigned int bit(unsigned int x) {
 return 1 << x;
}

Consider using std::variant

You have chosen a typical object-oriented solution to storing different event types inside EventBus. However, there is an alternative way to do this using std::variant. For example:

using Event = std::variant<KeyPressEvent, KeyReleaseEvent>;
struct EventBus {
public:
 void push_to_queue(Event&& event) {
 m_queue.emplace(event);
 }
 void process_queue() {
 while (!m_events.empty()) {
 const auto& event = m_events.front();
 for (auto& handler: m_handlers) {
 auto stop_propagation = handler->handle(event);
 if (stop_propagation) {
 break;
 }
 }
 m_events.pop();
 }
 }
 ...
private:
 std::list<IEventHandler*> m_handlers;
 std::queue<Event> m_queue;
};

A concrete handler could then look like:

struct Actor : public EventHandler {
 Actor(): EventHandler(EventType::KeyPressed | EventType::KeyReleased) { }
 bool handle(const Event& event) final {
 if (std::holds_alternative<KeyPressEvent>(event)) {
 std::cout << "Hey! You pressed a key!\n";
 }
 return true;
 }
};

• Matthieu M.: The advice on derived types is bizarre, Event has no virtual destructor, nor any virtual method, so clearly it should not be inherited from.
• G. Sliepen: @MatthieuM. Per the comments under OP's question, the idea is to add members to the derived class, and then presumably do a static_cast to the derived type in the handler to access them. But yes, it should have virtual destructors then. Or alternatively, std::variant should be used.