4
\$\begingroup\$

I need to have some thread-safe event handler (like C#'s EventHandler), so I made one. Here is my event handler class and sample code (compiled with VS2013). Is this code OK to be operated in multithreaded environment?

#include <functional>
#include <memory>
#include <atomic>
#include <list>
#include <conio.h>

////////////////////////////////////////////////////////////////////////////
// Event Handler class

template <typename _EventArg>
class EventHandler
{
public:
    EventHandler()
    {
        m_flag.clear();
    }

    EventHandler(const EventHandler& other)
    {
        m_flag.clear();
        m_ptr = other.m_ptr;
    }

    EventHandler& operator= (const EventHandler& other)
    {
        m_flag.clear();
        m_ptr = other.m_ptr;
        return *this;
    }

    virtual ~EventHandler()
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
    }

    template <typename _Type>
    EventHandler(void (_Type::*memFn)(_EventArg), _Type *obj)
    {
        m_flag.clear();
        m_ptr.reset(new std::function<void(_EventArg)>(std::bind(memFn, obj, std::placeholders::_1)));
    }

    void operator() (_EventArg e)
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
        if (m_ptr.get()) { (*m_ptr)(e); }
        m_flag.clear(std::memory_order_release);
    }

    bool operator== (const EventHandler& other)
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
        bool equal = (m_ptr == other.m_ptr);
        m_flag.clear(std::memory_order_release);
        return equal;
    }

private:
    std::atomic_flag m_flag;
    std::shared_ptr<std::function<void(_EventArg)>> m_ptr;
};

template <typename _Type>
class IEvent
{
public:
    IEvent()
    {
        m_flag.clear();
    }

    virtual ~IEvent()
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
    }

    void operator+= (const EventHandler<_Type>& handler)
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
        m_list.push_back(handler);
        m_flag.clear(std::memory_order_release);
    }

    void operator-= (const EventHandler<_Type>& handler)
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
        m_list.remove(handler);
        m_flag.clear(std::memory_order_release);
    }

    void operator() (_Type arg)
    {
        while (m_flag.test_and_set(std::memory_order_acquire));
        for (auto& f : m_list) { f(arg); }
        m_flag.clear(std::memory_order_release);
    }

private:
    std::atomic_flag m_flag;
    std::list<EventHandler<_Type>> m_list;

    // make non-copyable
    IEvent(const IEvent&);
    IEvent& operator= (const IEvent&);
};

////////////////////////////////////////////////////////////////////////////


// Event source class
class CSource {
public:
    void RaiseEvent(int value)
    {
        MyEvent(value); // Raise event
    }

    IEvent<int> MyEvent;
};

// Event receiver class
class CReceiver {
public:
    void MyHandler1(int e)
    {
        printf("CReceiver::MyHandler1 called with value %d\n", e);
    }

    void MyHandler2(int e)
    {
        printf("CReceiver::MyHandler2 called with value %d\n", e);
    }
};

void main()
{
    CSource source;
    CReceiver receiver;

    // define event handler
    EventHandler<int> handler1 = EventHandler<int>(&CReceiver::MyHandler1, &receiver);
    EventHandler<int> handler2 = EventHandler<int>(&CReceiver::MyHandler2, &receiver);

    // add event handler
    source.MyEvent += handler1;
    source.MyEvent += handler2;

    // raise event with value 1234!
    source.RaiseEvent(1234);

    // remove event handler
    source.MyEvent -= handler1;
    source.MyEvent -= handler2;

    _getch();
}
\$\endgroup\$
  • \$\begingroup\$ any reason for not using a proper mutex in favor of a busy loop lock? \$\endgroup\$ – ratchet freak Oct 22 '15 at 9:13
  • \$\begingroup\$ @ratchetfreak I heard atomic_flag is faster than mutex, so i used atomic_flag. \$\endgroup\$ – ooo0o Oct 22 '15 at 9:23
3
\$\begingroup\$

RAII

Whenever you need to acquire a resource, hold it for a while, and then release it deterministically - think resource acquisition is initialization. RAII! When I look at your equality comparison:

bool operator== (const EventHandler& other)
{
    while (m_flag.test_and_set(std::memory_order_acquire));
    bool equal = (m_ptr == other.m_ptr);
    m_flag.clear(std::memory_order_release);
    return equal;
}

We're really doing two things. We're acquiring and releasing a lock on m_flag, and then we're doing the equality comparison. So let's make that a lot more explicit.

std::lock_guard is an RAII lock, templated on a BasicLockable - which is just a class that supports a lock() and unlock(). So instead of holding a std::atomic_flag member, we can hold a member of type:

struct FlagMutex {
    void lock() {
        while (m_flag.test_and_set(std::memory_order_acquire));
    }

    void unlock() {
        m_flag.clear(std::memory_order_release);
    }

    /*
     * note that you were not initializing your flag correctly!
     */
    std::atomic_flag m_flag = ATOMIC_INIT_FLAG;
};

And then our equality operator becomes:

bool operator== (const EventHandler& other) {
    std::lock_guard<FlagMutex> lk(m_mutex);
    return m_ptr == other.m_ptr;
}

Keep that in mind throughout. Furthermore, if you do it this way, then FlagMutex can become a template argument for EventHandler so you can actually test the assertion that a std::atomic_flag is faster than a std::mutex.

Generalize Your Class

Right now, your event handler is constructible with:

template <typename _Type>
EventHandler(void (_Type::*memFn)(_EventArg), _Type *obj)

But why limit to just member functions? Your class has a std::function member. That can support free functions. Just generalize it.

Additionally, prefer make_shared to just new:

using FuncType = std::function<void(_EventArg)>;

template <typename F,
          typename = std::enable_if_t<
              std::is_constructible<FuncType(F&&)>::value
          >>
EventHandler(F&& func)
{
    m_ptr = std::make_shared<FuncType>(std::forward<F>(func));
}

The user can pass in a bind expression if they want, or a lambda, or any number of other things. Just lets you do more things.

Also _EventArg is a reserved name.

\$\endgroup\$
  • \$\begingroup\$ Thx for reply. // i heard std::function doesn't support compare operator(only for nullptr)... so i used shared_ptr for comparison. \$\endgroup\$ – ooo0o Oct 22 '15 at 17:30
  • \$\begingroup\$ @ooo0o Ah, good point, lemme fix that part. \$\endgroup\$ – Barry Oct 22 '15 at 17:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.