Thread-safe callback registration and invocation in C++11

Question

I need your help in reviewing the following code. My main concern is dead-lock, but there might also be other issues.

The EventGenerator object is owned by a single thread that running the ThreadTask. Other threads might access EventGenerator public APIs to set/remove event handlers.

A few corner cases that I've thought about:

1. The thread that is executing ThreadTask might call EventGenerator::{Add,Remove}Handler via the EventDelegate::Invoke.
2. The registered EventDelegate might be cancelled at any time. If EventDelegate::Cancel was called before the invocation, the invoke should not happen, if was called during/after, then nothing should happen.

#include <thread>
#include <atomic>
#include <mutex>
#include <iostream>
#include <vector>
#include <memory>
#include <chrono>

class IEventHandler {
public: virtual void onEvent(int data) = 0;
};

class EventDelegate {
private:
    IEventHandler* _handler;
    std::atomic<bool> _cancelled;
 public:
     EventDelegate(IEventHandler* h) : _handler(h), _cancelled(false) {};
     void Cancel() { _cancelled = true; }
     void Invoke(int data) { if (!_cancelled) _handler->onEvent(data); }
 };

 class EventGenerator {
 private:
     std::vector<std::shared_ptr<EventDelegate>> _handlers;
     std::mutex _mutex;
 public:
     void AddHandler(std::shared_ptr<EventDelegate> handler) {
         std::lock_guard<std::mutex> lck(_mutex);
         _handlers.push_back(handler);
     }
     void RemoveHanler(std::shared_ptr<EventDelegate> handler) {
         std::lock_guard<std::mutex> lck(_mutex);

         for (auto it = _handlers.begin(); it != _handlers.end(); ++it) {
             if ((*it) == handler) { _handlers.erase(it); break; }
         }
     }
     void ThreadTask() {
         int data = 0;

         while (true) {

             std::this_thread::sleep_for(std::chrono::seconds(1));
             data++;

             std::vector<std::shared_ptr<EventDelegate>> handlers_copy;

             {
                 std::lock_guard<std::mutex> lck(_mutex);
                 handlers_copy = _handlers;
             }

             for (auto& h : handlers_copy) { h->Invoke(data); }

             if (data == 15) { break; }

         }
    }
};


// Concrete class
class MyEventHandler : public IEventHandler {
public:
    virtual void onEvent(int data) {
        std::cout << "onEvent: data = " << data << std::endl;
    }
};


int main(int argc, char** argv) {
    EventGenerator generator;
    std::thread t(&EventGenerator::ThreadTask, &generator);

    MyEventHandler h;
    std::shared_ptr<EventDelegate> deleg = std::make_shared<EventDelegate>(&h);

    generator.AddHandler(deleg);
    std::this_thread::sleep_for(std::chrono::seconds(3));

    generator.RemoveHanler(deleg);
    std::this_thread::sleep_for(std::chrono::seconds(3));

    generator.AddHandler(deleg);
    std::this_thread::sleep_for(std::chrono::seconds(3));

    deleg->Cancel();
    std::this_thread::sleep_for(std::chrono::seconds(2));

    t.join();
    return 0;
}

Answer 1

From your concerns:

1. EventGenerator::{Add,Remove}Handler should be safe to call from EventDelegate::Invoke, though any updates won't be reflected in the current loop iteration in ThreadTask.
2. Your shared_ptrs are pointing to the same EventDelegate class, and you're using an atomic bool. This should behave properly.

My concerns are more minor. Your test code is too simple to catch many bugs (if they exist). Your ThreadTask function should use a for loop rather than while(true). You should consider using the remove/erase idiom in RemoveHanler. Finally, your code allows multiple copies of the same handler to be added. I don't know if that's intentional or not.

Answer 2

I will focus a bit more on RemoveHandler:

1. It is removing first handler (if multiple). Nothing bad about it, just to note that you can think about it. Using remove-erase could change that (removing all of them), I would not change the code in this way if you are fine with removing the first only (which makes sense, or possibly remove last not first).
2. The remove will be taken in to account in next round (when you copy handlers), not immediatelly. That is something that may be a problem. Think about adding Cancel() to RemoveHandler if that is desired behaviour (to cancel-remove it as soon as possible).

Alternatively you could track some current_index instead of making the copy, but that would complicate it a bit (you'll have to take care about it in RemoveHandler - possibly decrement it if index of removed is larger). It was just an idea, your copy is fine, no need to complicate it.