UDP Network server/client for gaming using boost.asio

Question

I've designed those classes for use in a multiplayer game with possibly very high number of clients for one server. Is this implementation good, or is there something obvious I overlooked or something to improve? I expect this part to have very high performance, so that I can just focus on other things and not come back to the basics too often.

Here are code listings:

NetworkServer.h

#pragma once

#include "Constants.h"
#include "locked_queue.h"

#include <boost/array.hpp>
#include <boost/asio.hpp>
#include <boost/bimap.hpp>
#include <boost/thread.hpp>

#include <string>
#include <array>

using boost::asio::ip::udp;

typedef boost::bimap<__int64, udp::endpoint> ClientList;
typedef ClientList::value_type Client;
typedef std::pair<std::string, __int64> ClientMessage;

class NetworkServer {
public:
    NetworkServer(unsigned short local_port);
    ~NetworkServer();

    bool HasMessages();
    ClientMessage PopMessage();

    void SendToClient(std::string message, unsigned __int64 clientID, bool guaranteed = false);
    void SendToAllExcept(std::string message, unsigned __int64 clientID, bool guaranteed = false);
    void SendToAll(std::string message, bool guaranteed = false);

    inline unsigned __int64 GetStatReceivedMessages() {return receivedMessages;};
    inline unsigned __int64 GetStatReceivedBytes()    {return receivedBytes;};
    inline unsigned __int64 GetStatSentMessages()     {return sentMessages;};
    inline unsigned __int64 GetStatSentBytes()        {return sentBytes;};

private:
    // Network send/receive stuff
    boost::asio::io_service io_service;
    udp::socket socket;
    udp::endpoint server_endpoint;
    udp::endpoint remote_endpoint;
    std::array<char, NetworkBufferSize> recv_buffer;
    boost::thread service_thread;

    void start_receive();
    void handle_receive(const boost::system::error_code& error, std::size_t bytes_transferred);
    void handle_send(std::string /*message*/, const boost::system::error_code& /*error*/, std::size_t /*bytes_transferred*/)    {}
    void run_service();
    unsigned __int64 get_client_id(udp::endpoint endpoint);

    void send(std::string pmessage, udp::endpoint target_endpoint);

    // Incoming messages queue
    locked_queue<ClientMessage> incomingMessages;

    // Clients of the server
    ClientList clients;
    unsigned __int64 nextClientID;

    NetworkServer(NetworkServer&); // block default copy constructor

    // Statistics
    unsigned __int64 receivedMessages;
    unsigned __int64 receivedBytes;
    unsigned __int64 sentMessages;
    unsigned __int64 sentBytes;
};

NetworkServer.cpp

#define _WIN32_WINNT 0x0501
#include <boost/bind.hpp>

#include "NetworkServer.h"
#include "Logging.h"

NetworkServer::NetworkServer(unsigned short local_port) :
    socket(io_service, udp::endpoint(udp::v4(), local_port)),
    nextClientID(0L),
    service_thread(std::bind(&NetworkServer::run_service, this))
{
    LogMessage("Starting server on port", local_port);
};

NetworkServer::~NetworkServer()
{
    io_service.stop();
    service_thread.join();
}

void NetworkServer::start_receive()
{
    socket.async_receive_from(boost::asio::buffer(recv_buffer), remote_endpoint,
        boost::bind(&NetworkServer::handle_receive, this, boost::asio::placeholders::error, boost::asio::placeholders::bytes_transferred));
}

void NetworkServer::handle_receive(const boost::system::error_code& error, std::size_t bytes_transferred)
{
    if (!error)
    {
        try {
            auto message = ClientMessage(std::string(recv_buffer.data(), recv_buffer.data() + bytes_transferred), get_client_id(remote_endpoint));
            if (!message.first.empty())
                incomingMessages.push(message);
            receivedBytes += bytes_transferred;
            receivedMessages++;
        }
        catch (std::exception ex) {
            LogMessage("handle_receive: Error parsing incoming message:",ex.what());
        }
        catch (...) {
            LogMessage("handle_receive: Unknown error while parsing incoming message");
        }
    }
    else
    {
        LogMessage("handle_receive: error: ", error.message());
    }

    start_receive();
}

void NetworkServer::send(std::string message, udp::endpoint target_endpoint)
{
    socket.send_to(boost::asio::buffer(message), target_endpoint);
    sentBytes += message.size();
    sentMessages++;
}

void NetworkServer::run_service()
{
    start_receive();
    while (!io_service.stopped()){
        try {
            io_service.run();
        } catch( const std::exception& e ) {
            LogMessage("Server network exception: ",e.what());
        }
        catch(...) {
            LogMessage("Unknown exception in server network thread");
        }
    }
    LogMessage("Server network thread stopped");
};

unsigned __int64 NetworkServer::get_client_id(udp::endpoint endpoint)
{
    auto cit = clients.right.find(endpoint);
    if (cit != clients.right.end())
        return (*cit).second;

    nextClientID++;
    clients.insert(Client(nextClientID, endpoint));
    return nextClientID;
};

void NetworkServer::SendToClient(std::string message, unsigned __int64 clientID, bool guaranteed) 
{
    try {
        send(message, clients.left.at(clientID));
    }
    catch (std::out_of_range) {
        LogMessage("Unknown client ID");
    }
};

void NetworkServer::SendToAllExcept(std::string message, unsigned __int64 clientID, bool guaranteed)
{
    for (auto client: clients)
        if (client.left != clientID)
            send(message, client.right);
};

void NetworkServer::SendToAll(std::string message, bool guaranteed)
{
    for (auto client: clients)
        send(message, client.right);
};

ClientMessage NetworkServer::PopMessage() {
    return incomingMessages.pop();
}

bool NetworkServer::HasMessages()
{
    return !incomingMessages.empty();
};

NetworkClient.h

#pragma once
#include "locked_queue.h"

#include <boost/array.hpp>
#include <boost/asio.hpp>
#include <boost/thread.hpp>

#include <memory>
#include <array>

#include "Constants.h"

using boost::asio::ip::udp;

class NetworkClient {
public:
    NetworkClient(std::string host, std::string server_port, unsigned short local_port = 0);
    ~NetworkClient();

    void Send(std::string message);
    inline bool HasMessages() {return !incomingMessages.empty();};
    inline std::string PopMessage() { if (incomingMessages.empty()) throw std::logic_error("No messages to pop"); return incomingMessages.pop(); };

    inline unsigned __int32 GetStatReceivedMessages(){return receivedMessages;};
    inline unsigned __int64 GetStatReceivedBytes(){return receivedBytes;};
    inline unsigned __int32 GetStatSentMessages(){return sentMessages;};
    inline unsigned __int64 GetStatSentBytes(){return sentBytes;};
private:
    // Network send/receive stuff
    boost::asio::io_service io_service;
    udp::socket socket;
    udp::endpoint server_endpoint;
    udp::endpoint remote_endpoint;
    std::array<char, NetworkBufferSize> recv_buffer;
    boost::thread service_thread;

    // Queues for messages
    locked_queue<std::string> incomingMessages;

    void start_receive();
    void handle_receive(const boost::system::error_code& error, std::size_t bytes_transferred);
    void run_service();

    NetworkClient(NetworkClient&); // block default copy constructor

    // Statistics
    unsigned __int32 receivedMessages;
    unsigned __int64 receivedBytes;
    unsigned __int32 sentMessages;
    unsigned __int64 sentBytes;
};

NetworkClient.cpp

#define _WIN32_WINNT 0x0501
#include <boost/bind.hpp>
#include <boost/thread.hpp>

#include "NetworkClient.h"
#include "Logging.h"

NetworkClient::NetworkClient(std::string host, std::string server_port, unsigned short local_port) : 
    socket(io_service, udp::endpoint(udp::v4(), local_port)), 
    service_thread(std::bind(&NetworkClient::run_service, this))
{
    receivedBytes = sentBytes = receivedMessages = sentMessages = 0;

    udp::resolver resolver(io_service);
    udp::resolver::query query(udp::v4(), host, server_port);
    server_endpoint = *resolver.resolve(query);
    Send("");
}

NetworkClient::~NetworkClient()
{
    io_service.stop();
    service_thread.join();
}

void NetworkClient::start_receive()
{
    socket.async_receive_from(boost::asio::buffer(recv_buffer), remote_endpoint,
        boost::bind(&NetworkClient::handle_receive, this, boost::asio::placeholders::error, boost::asio::placeholders::bytes_transferred)
    );
}

void NetworkClient::handle_receive(const boost::system::error_code& error, std::size_t bytes_transferred)
{
    if (!error)
    {
        std::string message(recv_buffer.data(), recv_buffer.data() + bytes_transferred);
        incomingMessages.push(message);
        receivedBytes += bytes_transferred;
        receivedMessages++;
    }
    else
    {
        LogMessage("NetworkClient::handle_receive:",error);
    }

    start_receive();
}

void NetworkClient::Send(std::string message)
{
    socket.send_to(boost::asio::buffer(message), server_endpoint);

    sentBytes += message.size();
    sentMessages++;
}

void NetworkClient::run_service()
{
    LogMessage("Client network thread started\n");
    start_receive();
    LogMessage("Client started receiving\n");
    while (!io_service.stopped()) {
        try {
            io_service.run();
        }
        catch (const std::exception& e) {
            LogMessage("Client network exception: ", e.what());
        }
        catch (...) {
            LogMessage("Unknown exception in client network thread");
        }
    }
    LogMessage("Client network thread stopped");
}

locked_queue class (#include "locked_queue.h") is a simple wrapper to a std::queue, locking access functions with a boost::mutex. For the reference:

#pragma once
#include <boost/thread/mutex.hpp>
#include <queue>
#include <list>

template<typename _T> class locked_queue
{
private:
    boost::mutex mutex;
    std::queue<_T> queue;
public:
    void push(_T value) 
    {
        boost::mutex::scoped_lock lock(mutex);
        queue.push(value);
    };

    _T pop()
    {
        boost::mutex::scoped_lock lock(mutex);
        _T value;
        std::swap(value,queue.front());
        queue.pop();
        return value;
    };

    bool empty() {
        boost::mutex::scoped_lock lock(mutex);
        return queue.empty();
    }
};

Feel free to use this code under the conditions of MIT license.

Update: The code updated according to review recommendations is available on GitHub.

Answer 1

Exception safety

Your locked_queue isn't exception safe. In particular:

    queue.pop();
    return value;

If the copy (or move) constructor for _T throws, you could have popped the item from the queue, then the constructor throws as you return the value, and the value is lost and can't be recovered. This is exactly why the standard library separates retrieving the value from removing the value from the collection--you copy first, then if (and only if) that succeeds, you remove it from the collection.

If you're sure you'll never use this with a type whose copy/move ctor can throw, what you're doing should be fine though. Unfortunately, you're using it with std::string, which has a copy ctor that can throw.

YAGNI

You have some of what might be termed "YAGNI violations". For example:

    void NetworkServer::SendToAll(std::string message, bool guaranteed)
    {
        for (auto client: clients)
            send(message, client.right);
    };

Here you're passing the function a bool, apparently to indicate whether it should attempt to guarantee delivery, but the function completely ignores that value. At least in normal use, such a guarantee basically is specified when you create the initial connection (TCP guarantees delivery, UDP doesn't).

bool parameters

You're using a bool as a parameter in a non-obvious way. I normally recommend against bool parameters in general. There are exceptions, but in this case it's not immediately obvious how SendToAll("whatever", true) and SendToAll("whatever", false) differ, and what the parameter is intended to mean (though, as noted above, in this case it means absolutely nothing).

Assuming you fix the code so the parameter means something, it would probably be better to replace the bool with an enum so the intent is directly visible in the code, something like this:

enum { ATTEMPT, GUARANTEE };

SendToAll("whatever", ATTEMPT);

...or, given C++11, you probably want to use an enum class instead:

enum class delivery { ATTEMPT, GUARANTEE };

SendToAll("whatever", delivery::ATTEMPT);

The same comment applies to SendToAllExcept.

member initializer lists

Prefer member initialization lists for initialization. An obvious example would be:

receivedBytes = sentBytes = receivedMessages = sentMessages = 0;

(inside the body of a ctor). In this case, you're probably better off with something like:

NetworkClient::NetworkClient(std::string host, std::string server_port, unsigned short local_port) : 
    socket(io_service, udp::endpoint(udp::v4(), local_port)), 
    service_thread(std::bind(&NetworkClient::run_service, this)),
    receivedBytes(0),
    sentBytes(0),
    receivedMessages(0),
    sentMessages(0)
{
    udp::resolver resolver(io_service);
    udp::resolver::query query(udp::v4(), host, server_port);
    server_endpoint = *resolver.resolve(query);
    Send("");
}

Alternatively, you might want to at least consider a small type specifically for counters:

class counter {
    size_t count;
public:
    counter &operator=(size_t val) { count = val; return *this; }
    counter(size_t count=0) : count(count) {}
    operator size_t() { return count; }
    count &operator++() { ++count; return *this; }
    count operator++(int) { counter ret(count); ++count; return ret; }
    bool operator==(counter const &other) { return count == other.count; }
    bool operator!=(counter const &other) { return count != other.count; }
};

With this, you simply define sentBytes, receivedBytes and so on as objects of the counter class, and it's impossible to create an object of that type that's not initialized.

prefer portability when possible

For one obvious example, you use unsigned __int64 in a number of places. __int64 is specific to VC++. Lacking a reason to do otherwise, I'd prefer to use unsigned long long, which works just as well on VC++, but also works with any other conforming implementation (of C++11). The counterpoint is that before C++11, some compilers (most notably VC++) did not support long long. If that's a concern, I'd use an intermediate typedef:

#ifdef _MSC_VER
typedef unsigned __int64 ulonglong;
#else
typedef unsigned long long ulonglong;
#endif

...then the rest of the code would use ulonglong. If you need more compiler-specific code than above, you'd still just change it in one place by adding a typedef for that specific compiler, with (hopefully) no changes necessary to the rest of the code.

const correctness

For example, you have four functions:

inline unsigned __int32 GetStatReceivedMessages(){return receivedMessages;};
inline unsigned __int64 GetStatReceivedBytes(){return receivedBytes;};
inline unsigned __int32 GetStatSentMessages(){return sentMessages;};
inline unsigned __int64 GetStatSentBytes(){return sentBytes;};

Since these functions should not modify the state of the object, they should probably be const member functions:

inline unsigned __int32 GetStatReceivedMessages() const {return receivedMessages;};
inline unsigned __int64 GetStatReceivedBytes() const {return receivedBytes;};
inline unsigned __int32 GetStatSentMessages() const {return sentMessages;};
inline unsigned __int64 GetStatSentBytes() const {return sentBytes;};

Consider more data grouping

For the same four functions as above, I'd prefer to create a stats class. The parent would return an object of that class, and then you'd query the class for the specific detail(s) you care about:

class stats {
    inline unsigned GetReceivedMessages() const { return receivedMessages; }
    // ...
};

class NetworkClient { 
    stats s;
public:
    stats Stats() const { return s; }
};

// ... and in client code, something like:
NetworkClient c;

std::cout << c.Stats().GetReceivedMessages();

Depending on the situation, you might also want to overload operator<< for that type so you can print out the stats even more easily:

 std::cout << c.Stats();

Avoid extraneous syntax

Although you're allowed to insert a semicolon after the body of a member function, it's unnecessary. IMO, it's better to leave it out, so (for example):

inline unsigned __int32 GetStatReceivedMessages() const {return receivedMessages;};

...becomes:

inline unsigned __int32 GetStatReceivedMessages() const { return receivedMessages; }

While I'm (a lot) less insistent about it than some people, I do think the extra white space helps readability in this case as well.