5
\$\begingroup\$

I always wanted to know how to pass member functions as arguments, and then I stumbled across templates that could automatically deduce types, and so I rejoiced! This is used in a Publisher-Subscriber system for a game server. I wanted the subscribing to individual packet 'types' (i.e. by enum) to be simple at the calling site.

/**
* Wrapper struct for callbacks fired when RakNet::Packet*s arrive.
* TargetObj is used to identify this callback by which object it is bound to.
*/
struct PacketCallback
{
    template <typename Obj>
    PacketCallback(void(Obj::*mf)(const RakNet::Packet*), Obj* obj)
        : TargetObj(obj)
    {
        Func = std::bind(mf, obj, std::placeholders::_1);
    }

    std::function<void(const RakNet::Packet*)> Func;
    void* TargetObj = nullptr;
};

Used thus:

class ClientManagementTest : public INetworkSubscriber
{
public:

    virtual void Subscribe(NetworkPublisher* a_networkPublisher) override
    {
        a_networkPublisher->Register(ID_NEW_INCOMING_CONNECTION,
            PacketCallback(&ClientManagement::OnClientConnected, this));

        a_networkPublisher->Register(ID_CONNECTION_LOST,
            PacketCallback(&ClientManagement::OnClientDisconnected, this));

        a_networkPublisher->Register(ID_DISCONNECTION_NOTIFICATION,
            PacketCallback(&ClientManagement::OnClientDisconnected, this));
    }


    virtual void Unsubscribe(NetworkPublisher* a_networkPublisher) override
    {
        a_networkPublisher->UnregisterAll(this);
    }

    void OnClientConnected(const RakNet::Packet* a_packet)
    {
        printf("Someone connected\n");
    }

    void OnClientDisconnected(const RakNet::Packet* a_packet)
    {
        printf("Someone disconnected\n");
    }

};

Are there any glaring flaws or possible improvements, here? Perhaps something that's incredibly unsafe?

For context, the function pointer is invoked thus, where the publisher stores the callbacks in the form std::map<int, std::vector<PacketCallback>> m_subscribers;:

void NetworkPublisher::NotifySubscribers(int a_packetType, RakNet::Packet* a_packet)
{
    auto it = m_subscribers.find(a_packetType);
    if (it != m_subscribers.end())
    {
        for (int i = 0; i < it->second.size(); ++i)
        {
            it->second[i].Func(a_packet);
        }
    }
}

I'm really happy with it, but would appreciate criticism.

Thanks!

\$\endgroup\$
4
\$\begingroup\$

Sorry for the brief answer, typing on phone.

Your NetworkPublisher should have Register(int signal, std::function<void()> callback)

And then simply do:

publisher.Register(WHATEVER_SIGNAL, std::bind(&MyClass::MySignalCallback, this));

This way you dont need the PacketCallback class and your subscriber interface is more generally usable as you can now pass lamdas and free functions as well. Further more your NotifySubscribers will simplify a bit.

Side note: You might want to consider using smart pointers instead of raw pointers. Or references if null is not an allowed argument.

Edit: In reply to comments, this is one way I would consider of doing it:

enum class SignalType{
   NewConnection,
   ConnectionLost
};

const std::array<SignalType,2> AllSignalTypes = {
    SignalType::NewConnection,
    SignalType::ConnectionLost};

class ISubscriber{
public:
    virtual bool supportsSignal(SignalType signalType) const = 0;
    virtual void signal(SignalType signalType, const RakNet::Packet& packet) = 0;
}

class Publisher{
public:
    void registerSubscriber(const std::shared_ptr<ISubscriber>& subscriber){
        for(auto& type : AllSignalTypes){
            if(subscriber->supportsSignal(type)){
                // TODO: Check for duplicates or change to std::unordered_set<weak_ptr>
                subscribers[type].emplace_back(subscriber);
            }
        }
    }

    void signal(SignalType signalType, const RakNet::Packet& packet){
        forEachLiveSubscriber(signalType, [&packet](const std::shared_ptr<ISubscriber>& subscriber){
            subscriber->signal(signalType, packet);
            return true;
        });
    }

    void unregisterAll(const ISubscriber& subscriber){
        // Technically not necessary but for good measure
        for(auto& type : AllSignalTypes){
            forEachLiveSubscriber(signalType, [&packet](const std::shared_ptr<ISubscriber>& sharedSubscriber){
                return sharedSubscriber.get() != &subscriber;
            });
        }
    }

private:
    std::unordered_map<SignalType, std::vector<std::weak_ptr<ISubscriber>>> subscribers;

    template<typename Callable>
    void forEachLiveSubscriber(SignalType signalType, Callable&& callback){
        auto& signalSubscribers = subscribers[signalType];

        auto it = signalSubscribers.begin();
        while(it != signalSubscribers.end()){
            auto& strongPtr = it->lock();
            if(strongPtr && callback(strongPtr)){
                it++;
            }
            else{
                // The subscriber has destructed without unregistering,
                // Or the callback requested us to remove it.
                it = signalSubscribers.erase(it);
            }
        }
    }
}

class MySubscriber : public ISubscriber{
    bool supportsSignal(SignalType signalType) const override{
        return signalType == NewConnection || signalType == ConnectionLost;
    }

    void signal(SignalType signalType, const RakNet::Packet& packet) override{
        switch(signalType){
            case NewConnection:
                std::cout<<"Someone connected"<<std::endl;
                break;
            case ConnectionLost:
                std::cout<<"Someone lost connection"<<std::endl;
                break;
        }
    }
}

Note that by using shared_ptr we get access to weak_ptr which in turn allows lazy garbage collection of the subscribers. In turn this means that a subscriber doesn't have to unregister if all shared pointers to it just simply die.

The above code is untested pseudocode. Some assembly may be required :)

\$\endgroup\$
  • \$\begingroup\$ Hi Emily, thanks for the advice! Part of the reason I used the struct was so I could store the obj alongside the function pointer, so I could do this. Is it doable some other way? std::function doesn't seem to support pulling the obj ptr back out. Re. raw pointers - just following how games libraries and engines do it. RakNet gives me the Packet* pointer raw :) \$\endgroup\$ – HateDread Dec 4 '16 at 20:26
  • \$\begingroup\$ @HateDread Re. raw pointers. Simply insert the raw pointer into a shared_ptr or unique_ptr with a suitable Deleter. I will edit with an update for your example. \$\endgroup\$ – Emily L. Dec 5 '16 at 10:39
2
\$\begingroup\$

PacketCallback

Encapsulation - currently anyone can modify either member, and only the intended functionality should be exposed, e.g.

class PacketCallback
{
public:
    template <typename Obj>
    PacketCallback(void(Obj::*mf)(const RakNet::Packet*), Obj* obj)
        : TargetObj(obj)
    {
        Func = std::bind(mf, obj, std::placeholders::_1);
    }

    bool isTarget(void* other) 
    {
        return other == TargetObj;
    }

    void operator()(const RakNet::Packet* packet)
    {
        Func(packet);
    }

private:
    std::function<void(const RakNet::Packet*)> Func;
    void* TargetObj = nullptr;
};

NetworkPublisher

Rather than using a map of vectors, you could instead use a multimap

std::multimap<int, PacketCallback> m_subscribers;

void NetworkPublisher::NotifySubscribers(int a_packetType, RakNet::Packet* a_packet)
{
    auto rng = m_subscribers.equal_range(a_packetType);
    for (auto it = rng.first; it != rng.second; ++it)
    {
        (*it)(a_packet);
    }

}
\$\endgroup\$
  • \$\begingroup\$ A range-for would make that last part much nicer: for (auto &sub : rng) { sub(a_packet); } \$\endgroup\$ – Edward Dec 5 '16 at 15:24
  • \$\begingroup\$ Yes, although a pair of iterators isn't directly usable in a range-for, something like for (auto &sub : boost::make_iterator_range(rng) {...} \$\endgroup\$ – Caleth Dec 5 '16 at 15:59
0
\$\begingroup\$

One important thing not mentioned yet in the other reviews is that this is likely to be quite unsafe because there's no assurance that when a callback is invoked that the associated object still exists. For example:

NetworkPublisher nwp;

int badSubscribe() {
    ClientManagementTest cmt;   // create on stack
    cmd.subscribe(&nwp);        // subscribe to global nwp
} // cmt is destroyed, but the callbacks still exist

// later, when a RakNet::Packet is received,
// member functions of the no longer existing cmt are invoked
// which is undefined behavior

The way to deal with this is to assure that the object will continue to exist when the callback is invoked, and one way to assure that is to use enable_shared_from_this.

Specifically, one would derive ClientManagementTest from std::enable_shared_from_this and then return a std::shared_ptr to the object rather than simply this or a pointer to the member function.

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