5
\$\begingroup\$

This is an event system implementation for C++.

GitHub

I'm targeted on following features:

  1. RAII-friendly
  2. any number of event parameters
  3. ability to use member functions, free functions and lambdas as well
  4. almost c# like usage

Now I'm looking for review, ideas and critique.

The current solution for event handlers identification is more unpleasant thing for now, in my opinion.


Event subscribers should be derived from EventReceivable: EventReceivable.h

#ifndef EVENTRECEIVABLE_H
#define EVENTRECEIVABLE_H

#include <memory>
#include <atomic>

class EventReceivable
{
public:
    class EventReceiver
    {
    public:
        explicit EventReceiver(EventReceivable* instance);

        ~EventReceiver() = default;

    public:
        auto instance() const -> EventReceivable* { return instance_; }

        void instance(EventReceivable* instance) { instance_ = instance; }

    private:
        EventReceivable* instance_;
    };

public:
    EventReceivable(EventReceivable const& eventReceivable) = delete;

public:
    EventReceivable& operator=(EventReceivable const& rhs) = delete;

public:
    auto eventReceiver() const -> std::shared_ptr<EventReceiver> const& { return eventReceiver_; }

    auto id() const -> uint_fast64_t { return id_; }

protected:
    EventReceivable();

    EventReceivable(EventReceivable &&eventReceivable) noexcept;

    ~EventReceivable() = default;

protected:
    EventReceivable& operator=(EventReceivable&& rhs) noexcept;

private:
    std::uint_fast64_t              id_;
    std::shared_ptr<EventReceiver>  eventReceiver_;

private:
    static std::atomic_uint_fast64_t lastId_;
};

#endif //PROJECT_EVENTRECEIVABLE_H

EventReceivable.cpp

#include "EventReceivable.h"

std::atomic_uint_fast64_t EventReceivable::lastId_{0};

EventReceivable::EventReceiver::EventReceiver(EventReceivable* instance) :
    instance_{instance}
{
}

EventReceivable::EventReceivable() :
    id_{++lastId_}
    , eventReceiver_{std::make_shared<EventReceiver>(this)}
{
}

EventReceivable::EventReceivable(EventReceivable &&eventReceivable) noexcept :
    id_{eventReceivable.id_}
    , eventReceiver_{std::move(eventReceivable.eventReceiver_)}
{
    eventReceivable.id_ = 0;
    eventReceiver_->instance(this);
}

EventReceivable& EventReceivable::operator=(EventReceivable&& rhs) noexcept {
    eventReceiver_ = std::move(rhs.eventReceiver_);
    eventReceiver_->instance(this);

    id_ = rhs.id_;
    rhs.id_ = 0;

    return *this;
}

Event class: Event.h

#ifndef EVENT_H
#define EVENT_H

#include <functional>
#include <unordered_map>
#include <algorithm>
#include <utility>
#include <boost/functional/hash.hpp>
#include "EventReceivable.h"

template<typename... Args>
class Event
{
private:
    struct Dummy {
        void func() {};
        virtual void vFunc() {};
    };

    using EventHandlerIdentifier = std::array<
            std::byte, sizeof(uint_fast64_t) + std::max(sizeof(&Dummy::func), sizeof(&Dummy::vFunc))>;

public:
    class EventHandler
    {
    friend Event;

    public:
        template<class T, class M>
        EventHandler(T *instance, void (M::*member)(Args...)) :
            isStatic_{false}
            , identifier_{}
            , eventReceiver_{instance->eventReceiver()}
            , handler_{}
        {
            auto id = instance->id();

            std::memset(identifier_.data(), 0, identifier_.size());
            std::memcpy(identifier_.data(), &id, sizeof(uint_fast64_t));
            std::memcpy(identifier_.data() + sizeof(uint_fast64_t), &member, sizeof(member));

            auto eventReceiver = eventReceiver_;

            handler_ = [eventReceiver, member](auto&&... args) -> void {
                assert(!eventReceiver.expired());

                auto receiver = eventReceiver.lock();

                ((static_cast<T*>(receiver->instance()))->*member)(std::forward<decltype(args)>(args)...);
            };
        }

        template<typename SFunc>
        EventHandler(SFunc const& staticHandler) :
            isStatic_{true}
            , identifier_{}
            , eventReceiver_{}
            , handler_{[staticHandler](auto&&... args) -> void {
                staticHandler(std::forward<decltype(args)>(args)...);
            }}
        {
            std::memset(identifier_.data(), 0, identifier_.size());
            std::memcpy(identifier_.data() + sizeof(uint_fast64_t), &staticHandler, sizeof(uintptr_t));
        }

        EventHandler(EventHandler const&) = default;

        EventHandler(EventHandler&&) = default;

        ~EventHandler() = default;

    public:
        EventHandler &operator=(EventHandler const&) = default;

        EventHandler &operator=(EventHandler&&) = default;

        template<typename... EventArgs>
        void operator()(EventArgs&&... args) {
            invoke(std::forward<EventArgs>(args)...);
        }

        explicit operator bool() const {
            return isStatic_ ? true : !eventReceiver_.expired();
        }

    public:
        template<typename... EventArgs>
        void invoke(EventArgs&&... args) {
            handler_(std::forward<EventArgs>(args)...);
        }

    public:
        auto identifier() const -> EventHandlerIdentifier const& { return identifier_; }

    private:
        bool                                            isStatic_;

        EventHandlerIdentifier                          identifier_;

        std::weak_ptr<EventReceivable::EventReceiver>   eventReceiver_;

        std::function<void(Args...)>                    handler_;
    };

public:
    Event() : eventHandlers_{} { }

    Event(Event const&) = delete;

    Event(Event&&) = default;

    ~Event() = default;

public:
    Event& operator=(Event const&) = delete;

    Event& operator=(Event&&) = default;

    template<typename Handler>
    auto operator+=(Handler const& rhs) -> EventHandler
    {
        static_assert(std::is_same_v<EventHandler, std::decay_t<Handler>> || std::is_invocable_v<Handler, Args...>);

        if constexpr (std::is_same_v<EventHandler, std::decay_t<Handler>>) {
            eventHandlers_.emplace(rhs.identifier(), rhs);
            return rhs;
        } else {
            EventHandler eh{rhs};
            eventHandlers_.emplace(eh.identifier(), eh);
            return eh;
        }
    }

    void operator-=(EventHandler const& rhs)
    {
        eventHandlers_.erase(rhs.identifier());
    }

    template<typename... EventArgs>
    void operator()(EventArgs&&... args)
    {
        auto it = eventHandlers_.begin();

        while (it != eventHandlers_.end()) {
            EventHandler& eventHandler = (*it).second;

            if (eventHandler) {
                eventHandler(std::forward<EventArgs>(args)...);
                it++;
            }
            else {
                it = eventHandlers_.erase(it);
            }
        }
    }

private:
    std::unordered_map<EventHandlerIdentifier, EventHandler, boost::hash<EventHandlerIdentifier>> eventHandlers_;
};
#endif //PROJECT_EVENT_H

Usage examples:

class Emitter {
public:
    Event<uint32_t> event;
}

class Receiver final: public EventReceivable {
public:
    void onEvent(uint32_t val) { result_ = val; }

    auto result() const { return result_; } 
private:
    uint32_t result_ = 0;
}

Emitter  emitter;
Receiver receiver1;

// ordinary usage:
emitter.event += Event<uint32_t>::EventHandler(&receiver1, &Receiver::onEvent);

// RAII test:
{
Receiver receiver2;

emitter.event += Event<uint32_t>::EventHandler(&receiver2, &Receiver::onEvent);

// do not care if receiver is still alive or deleted
// emitter.event -= Event<uint32_t>::EventHandler(&receiver2, 
// &Receiver::onEvent);
}

// can use lambdas:
uint32_t result = 0;
emitter.event += [&](uint32_t val) { result = val; };

emitter.event(42);
\$\endgroup\$
3
\$\begingroup\$
public:
public:
public:

and double-spacing is not normal C++ style.


I see you are generally using up-to-date C++ library and language features. But, the handler should be a std::function rather than a void (M::*member)(Args...). That makes me wonder if this class is doing a lot of the heavy lifting that std::function already does, including local buffer optimizations? Could your class be built around that instead?


The memcpy/memset use of identifier.data makes me want to look closer at that. Could it be done in a properly typed manner? And since it’s a std::array, you could use std::fill_n instead of memset.


Your constructor is listing various members initializers with no arguments.

: identifier_{}, handler_{}

shouldn’t that be what normally happens if you don’t list it at all?

\$\endgroup\$
  • \$\begingroup\$ On your last point, g++ -Weffc++ does recommend writing initializers, even if the default would be the same. It does show that you didn't forget about them, I guess. \$\endgroup\$ – Toby Speight Apr 20 '18 at 8:55
  • \$\begingroup\$ The points that i'm agreed with: 1. "you could use std::fill_n instead of memset" 2. "Your constructor is listing various members initializers with no arguments" - yes, i can exclude them from list, but i usually keeps all class members in list, it just helps me to be sure, that initialization order is correct. 3. and i'm appreciated code-style notes as well \$\endgroup\$ –  BAntDit Apr 20 '18 at 9:31
  • \$\begingroup\$ I don't agree about std::function and templates. In case i have choise between std::functions and templates - i always prefer templates. In this case i also wrap user handler into lambda to track if receiver object is still alive, so in case i would use std::function, i've got in situation when std::function store lambda that catch another std::function that wrap actual call and has overhead for virtual lookups... \$\endgroup\$ –  BAntDit Apr 20 '18 at 9:33
1
\$\begingroup\$

Bug:

            assert(!eventReceiver.expired());

            auto receiver = eventReceiver.lock();

            ((static_cast<T*>(receiver->instance()))->*member)(std::forward<decltype(args)>(args)...);

This is a race condition as the assert is not going to prevent the lock on the next line from failing. The state may change between these two lines.

You should write the test on the result of the call to lock.

if (auto receiver = eventReceiver.lock())
    // go ahead and use receiver as a pointer
else
    // receiver is gone.

Also, what’s normally done in this kind of async callback case is to simply ignore the callback (and mark it for removal) if it is found to be stale, rather than asserting an error. The assert doesn’t even do anything in a release build, anyway, so you’ll just get a null pointer error when you dereference it. The possibility of the object being destroyed while callbacks are in queues somewhere is the whole point of having this kind of system, right?


Problems

I’ve looked over the code in more detail, and I see what is the central idea: Your receiver object is not necessarily controlled directly by a shared_ptr, but may be used in any manner, such as a subobject or local object. So, a proxy is created which points back to this object, which is itself controlled by a shared_ptr. The destructor of the object drops the reference on the proxy which renders subsequent events able to detect that it is no longer there.

However, this will not work. Consider:

Receiver Object destructor is called.

It goes on its merry way tearing down the Object, all the while the EventReceivable’s shared_ptr is still unchanged, so events processed on other threads or recursively will hit Receiver Object while it is in a bad state.

The inherited EventReceivable is destroyed last, and it drops the reference count.

Meanwhile, suppose that an even is still being executed on another thread. It is holding the reference count, so the EventReceiver is still alive, still pointing to the now defunct ReceiverObject which has had its memory reused for something else. Another event being processed, perhaps on a third thread or as a nested event to handle, sees the Event Receiver is still alive and proceeds to use the bad pointer.

The point is that you cannot use a proxy lifetime manager stand-in like this; you must reflect and affect the actual lifetime of the underlying object!


Conceptual thoughts

Your are setting things up to handle a member function call on a receiver object. Generally, a callback may be a free function, and may involve one or more objects with lifetimes to be managed. It may well be a function of yet another class, or a free function written just for the use of this callback.

Given (as discussed above) that you must use actual shared_ptr’s to the objects in question, there is no reason to intrusively adapt the objects that may be called back (e.g. your required base class).

I see such a callback can be handled idiomatically like this:

auto callback =
    [ wp1= get_weak(thing1), wp2= get_weak(thing2) ]
    ( int code )
    {
        auto p1= wp1.lock();
        if (!p1) return;
        auto p2= wp2.lock();
        if (!p2) return;

        call_the_thing (arg1, 42 *p1, *p2, code, whatever);
     }

auto ticket= some_object.on_event(std::move(callback));

The ticket is an id of some kind assigned by the even manager on the object that you registered the callback on; you use this if you want to revoke the callback.

some_object.revoke(ticket);

At its simplest, it’s just a number assigned per-object. But I’ve also made systems where this was an RAII object and destroying the ticket would revoke the callback.


※ In the past, I’ve implemented something like bind that handled weak_ptr and resolved them in order to invoke the function. With C++17 getting this done is easy, as shown here using a lambda and explicit capture initialization.

\$\endgroup\$

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.