Thread-safe callback registration and invocation in C++11 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:


The thread that is executing ThreadTask might call EventGenerator::{Add,Remove}Handler via the EventDelegate::Invoke.
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 &lt;thread&gt;
#include &lt;atomic&gt;
#include &lt;mutex&gt;
#include &lt;iostream&gt;
#include &lt;vector&gt;
#include &lt;memory&gt;
#include &lt;chrono&gt;

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

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

 class EventGenerator {
 private:
     std::vector&lt;std::shared_ptr&lt;EventDelegate&gt;&gt; _handlers;
     std::mutex _mutex;
 public:
     void AddHandler(std::shared_ptr&lt;EventDelegate&gt; handler) {
         std::lock_guard&lt;std::mutex&gt; lck(_mutex);
         _handlers.push_back(handler);
     }
     void RemoveHanler(std::shared_ptr&lt;EventDelegate&gt; handler) {
         std::lock_guard&lt;std::mutex&gt; 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&lt;std::shared_ptr&lt;EventDelegate&gt;&gt; handlers_copy;

             {
                 std::lock_guard&lt;std::mutex&gt; lck(_mutex);
                 handlers_copy = _handlers;
             }

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

             if (data == 15) { break; }

         }
    }
};


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


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

    MyEventHandler h;
    std::shared_ptr&lt;EventDelegate&gt; deleg = std::make_shared&lt;EventDelegate&gt;(&amp;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-&gt;Cancel();
    std::this_thread::sleep_for(std::chrono::seconds(2));

    t.join();
    return 0;
}
From your concerns:


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.
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.