Handler for incoming network packages

Question

I want to let you look through my NetHandler which is basically the component which gets notified when a full packet arrived (not like tcp-packet but my own type of packets) and handles it.

First of all, the handle method which gets called from the networking

void NetHandler::handle(int id, char* data, int len) {
#define P(ID, TYPE) case ID: try_handle<TYPE>(data, len); break
    switch (id) {
        P(0, game::networking::packets::Hello);
        P(1, game::networking::packets::Disconnect);
        P(2, game::networking::packets::Chat);
    default:
        unhandled(id);
    }
#undef P
}

Yeah... As you can see, a little define to create a case for me. unhandled just prints out an error.

Next, the try_handle<>()

template<typename T>
void try_handle(char* data, int len) {
    T packet;
    if (!packet.ParseFromArray(data, len)) {
        bad_packet(packet.GetTypeName(), len);
        return;
    }
    handle(packet);
}

It creates the actual corresponding Packet and parses it (I use google protobuf). bad_packet, again, just prints an error. Now the handle

#define STUB(TYPE) virtual void handle(TYPE& type) { unhandled(TYPE::default_instance().GetTypeName()); }
    STUB(game::networking::packets::Hello)
    STUB(game::networking::packets::Disconnect)
    STUB(game::networking::packets::Chat)
#undef STUB

I create a virtual method for every packet-type which calls unhandled. In my application I can now subclass this NetHandler and override the handler-methods for all packets I expect.

Now the other direction, send packets. I actually wanted a single send-method which takes every possible packet and just sends it. But it needs to send the id along with it, so I created a PacketRegistry

class PacketRegistry {
public:
    PacketRegistry() {
        for (int i = 0; i < 256; i++)
            messages[i] = nullptr;
        reg<game::networking::packets::Hello>(0);
        reg<game::networking::packets::Disconnect>(1);
        reg<game::networking::packets::Chat>(2);
    }

    template<typename T>
    void reg(int id) {
        messages[id] = T::default_instance().GetDescriptor();
    }

    int getId(const google::protobuf::Message& message) {
        const google::protobuf::Descriptor* ptr = message.GetDescriptor();
        for (int i = 0; i < 256; i++) {
            if (messages[i] == ptr)
                return i;
        }
        return -1;
    }

    const google::protobuf::Descriptor* messages[256];
};

As you can see, this is the third time I need to add every packet manually.

I tried to come up with some template-magic, but I'm not that experienced yet. Something like

//in NetHandler
template<typename T>
void handle(T& packet) { unhandled(T::default_instance().GetTypeName()); }

//In a subclass
template<>
void handle<game::networking::packets::Chat>(game::networking::packets::Chat& chat) {   }

unfortunately doesn't work because... well... templates and virtual don't really like each other.

Any ideas how I can let the template-magic do more of the dirty work instead of defining methods, cases, and so on for every existing packet?

Answer 1

Few suggestions:

• Add/use inheritance for packet type (assuming you have something like game::networking::packets::PacketTypeBase that all packet types inherit from. Have GetDescriptor()/GetTypeName()/handle() on this base class

• Create a factory to go from int id to packet Type. Something like: game::networking::packets::PacketTypeBase make_packet(int id);

• Add a virtual function handleInput(char* data, int len) so your NetHandler::handle simply becomes return make_packet(id).handleInput(data,len). The default can be handled by base class or separate UnknownPacket child class.

I would recommend using inheritance on packet type, not NetHandler. If you need flexibility, you can even pass a functor into the ctor of NetHandler if you want different work done on the PacketType by each instance of NetHandler

In general try to replace switch statement with inheritance and virtual functions. Factory/maker function comes in handy even during testing. Common functionality can be put in base class so you can avoid duplicate code (not need macros) & still do packet specific work.

Instead of having a hard-coded list of 256 (avoid magic numbers), use a std::map / std::unordered_map/HashMap so find / lookup will be faster in getId (use a data structure that optimizes your most common case)

Hope this helps.