Sockets and networking for a multi-platform game

Question

I'm in the process of adding network support for a little Mac & Windows game I'm writing. Today I've finished the basic sockets plus a few other network helpers and would appreciate having the code reviewed.

Networking/Sockets.hpp:

#pragma once

#ifdef _WIN32
    // Windows, any version:
    #include <WinSock2.h>
    typedef SOCKET NetSocketId;
    #define NET_INVALID_SOCKET_ID INVALID_SOCKET
    #define NET_SOCKET_ERROR      SOCKET_ERROR
#else // !_WIN32
    // Assume a Unix-like system:
    #include <sys/types.h>
    #include <sys/socket.h>
    #include <unistd.h>
    #include <fcntl.h>
    #include <netdb.h>
    #include <errno.h>
    #include <netinet/in.h>
    #include <arpa/inet.h>
    typedef int NetSocketId;
    #define NET_INVALID_SOCKET_ID (-1)
    #define NET_SOCKET_ERROR      (-1)
#endif // _WIN32

#include <stdexcept>
#include <string>

namespace Engine {

    // ======================================================
    // Socket exception type:
    // ======================================================

    class SocketError
        : public std::runtime_error {

    public:

        SocketError(const std::string & message);
    };

    // ======================================================
    // Socket base class:
    // ======================================================

    class Socket {

    public:

        // Underlaying socket connection types:
        enum class ConnectionType { Blocking, NonBlocking };

        // Construction / destructor:
        explicit Socket(NetSocketId sid);
        virtual ~Socket();

        // Copy is disallowed.
        Socket(const Socket & other) = delete;
        Socket & operator = (const Socket & other) = delete;

        // Is movable.
        Socket(Socket && other) noexcept;
        Socket & operator = (Socket && other) noexcept;

        // Read and write bytes to the socket stream:
        void ReceiveBytes(void * buffer, size_t numBytes) const;
        void SendBytes(const void * buffer, size_t numBytes) const;

        // Manually close a connection.
        // Note: This is done automatically by the destructor.
        void Close();

    protected:

        Socket();

        // Underlaying socket handle, specific to the platform.
        NetSocketId socketId;

    private:

        // Shared states management.
        // Network is initialized when the fist socket is created
        // and terminated when the last connection is closed.
        static void NetInit();
        static void NetShutdown();
        static int  numAliveSockets;
    };

    using SocketPtr = std::unique_ptr<Socket>;

    // ======================================================
    // ClientSocket:
    // ======================================================

    class ClientSocket
        : public Socket {

    public:

        ClientSocket(const std::string & hostNameOrIpAddress, const HostShort & portNumber);
    };

    using ClientSocketPtr = std::unique_ptr<ClientSocket>;

    // ======================================================
    // ServerSocket:
    // ======================================================

    class ServerSocket
        : public Socket {

    public:

        ServerSocket(const HostShort & portNumber, int pendingQueueSize, Socket::ConnectionType type);

        // Returns null if this is a non-blocking socket and there are no pending
        // connections. Block and wait for a connection if this is a blocking socket.
        SocketPtr AcceptConnection() const;
    };

    using ServerSocketPtr = std::unique_ptr<ServerSocket>;

} // namespace Engine {}

Networking/Sockets.cpp:

// Engine core:
#include "Engine/Core/Utilities.hpp"
#include "Engine/Core/Common.hpp"

// Networking:
#include "Engine/Networking/NetCommon.hpp"
#include "Engine/Networking/Sockets.hpp"

// This will automatically add the network
// library for a Windows build on Visual Studio.
#ifdef _MSC_VER
    #pragma comment(lib, "Ws2_32.lib")
#endif

namespace Engine {

// ======================================================
// Socket exception type:
// ======================================================

SocketError::SocketError(const std::string & message)
    #ifdef _WIN32
    : std::runtime_error(message + std::string(" | sys error: ") + std::to_string(WSAGetLastError()))
    #else // !_WIN32
    : std::runtime_error(message + std::string(" | sys error: ") + std::strerror(errno))
    #endif // _WIN32
{
    // Socket error message will be formated as:
    //   user message | sys error: return from strerror(errno) or WSAGetLastError()
    //
    // List of WSA error codes:
    //   http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx
}

// ======================================================
// Static network initialization:
// ======================================================

int Socket::numAliveSockets = 0;

void Socket::NetInit()
{
    if (numAliveSockets == 0)
    {
        common->PrintF("-------- Socket::NetInit() ---------");

        #ifdef _WIN32
        WSADATA info;
        if (WSAStartup(MAKEWORD(2, 0), &info) != 0)
        {
            throw SocketError("Couldn't start WSA!");
        }
        #endif // _WIN32
    }

    ++numAliveSockets;
}

void Socket::NetShutdown()
{
    assert(numAliveSockets == 0);
    common->PrintF("------ Socket::NetShutdown() -------");

    #ifdef _WIN32
    WSACleanup();
    #endif // _WIN32
}

// ======================================================
// Socket:
// ======================================================

Socket::Socket()
    : socketId(NET_INVALID_SOCKET_ID)
{
    NetInit();

    // `AF_INET` is for IPv4.
    // UDP would use `SOCK_DGRAM` instead of `SOCK_STREAM`.
    //
    // About the last parameter `protocol`:
    //   "If a value of 0 is specified, the caller does not wish to specify
    //   a protocol and the service provider will choose the protocol to use."
    //
    socketId = socket(AF_INET, SOCK_STREAM, /* protocol = */ 0);
    if (socketId == NET_INVALID_SOCKET_ID)
    {
        throw SocketError("Failed to create socket with 'socket(AF_INET, SOCK_STREAM, 0)'!");
    }
}

Socket::Socket(const NetSocketId sid)
    : socketId(NET_INVALID_SOCKET_ID)
{
    NetInit();
    socketId = sid;
}

Socket::Socket(Socket && other) noexcept
{
    socketId       = other.socketId;
    other.socketId = NET_INVALID_SOCKET_ID;
}

Socket & Socket::operator = (Socket && other) noexcept
{
    socketId       = other.socketId;
    other.socketId = NET_INVALID_SOCKET_ID;
    return *this;
}

Socket::~Socket()
{
    Close();

    // Finalize the network when the last connection closes.
    if (--numAliveSockets == 0)
    {
        NetShutdown();
    }
}

void Socket::Close()
{
    if (socketId != NET_INVALID_SOCKET_ID)
    {
        int result;

        #ifdef _WIN32
        result = closesocket(socketId);
        #else // !_WIN32
        result = close(socketId);
        #endif // _WIN32

        if (result == 0)
        {
            common->PrintF("Closed socket connection...");
        }
        else
        {
            common->WarningF("Failed to close a socket connection!");
        }

        socketId = NET_INVALID_SOCKET_ID;
    }
}

void Socket::ReceiveBytes(void * buffer, const size_t numBytes) const
{
    assert(socketId != NET_INVALID_SOCKET_ID);
    assert(buffer   != nullptr);
    assert(numBytes != 0);

    const long result = recv(socketId, buffer, numBytes, 0);

    if (result == NET_SOCKET_ERROR)
    {
        throw SocketError("Failed to 'recv()' bytes! NET_SOCKET_ERROR!");
    }
    else if (static_cast<size_t>(result) != numBytes)
    {
        throw SocketError("Failed to 'recv()' bytes! Not enough bytes read!");
    }
    // Successful recv().
}

void Socket::SendBytes(const void * buffer, const size_t numBytes) const
{
    assert(socketId != NET_INVALID_SOCKET_ID);
    assert(buffer   != nullptr);
    assert(numBytes != 0);

    const long result = send(socketId, buffer, numBytes, 0);

    if (result == NET_SOCKET_ERROR)
    {
        throw SocketError("Failed to 'send()' bytes! NET_SOCKET_ERROR!");
    }
    else if (static_cast<size_t>(result) != numBytes)
    {
        throw SocketError("Failed to 'send()' bytes! Couldn't send all the data!");
    }
    // Successful send().
}

// ======================================================
// ClientSocket:
// ======================================================

ClientSocket::ClientSocket(const std::string & hostNameOrIpAddress, const HostShort & portNumber)
{
    // Should have been initialized by the superclass.
    assert(socketId != NET_INVALID_SOCKET_ID);

    // Must have an IP address or URL.
    assert(!hostNameOrIpAddress.empty());

    //
    // Note: `gethostbyname()` is deprecated in WinSock2 in favor
    // of `getaddrinfo()`. So this is a possible future improvement.
    //
    hostent * hostEntity = gethostbyname(hostNameOrIpAddress.c_str());
    if (hostEntity == nullptr)
    {
        Close();
        throw SocketError("Failed to get hostent for " + hostNameOrIpAddress);
    }

    sockaddr_in addr;
    std::memset(&addr, 0, sizeof(addr));

    addr.sin_family = AF_INET;
    addr.sin_port   = portNumber.ToNetShort().GetRawValue();
    addr.sin_addr   = *reinterpret_cast<in_addr *>(hostEntity->h_addr);

    if (connect(socketId, reinterpret_cast<sockaddr *>(&addr), sizeof(addr)) != 0)
    {
        Close();
        throw SocketError("Failed to connect with host " + hostNameOrIpAddress +
                " at port #" + std::to_string(portNumber.GetRawValue()));
    }
}

// ======================================================
// ServerSocket:
// ======================================================

ServerSocket::ServerSocket(const HostShort & portNumber, const int pendingQueueSize, const Socket::ConnectionType type)
{
    // Should have been initialized by the superclass.
    assert(socketId != NET_INVALID_SOCKET_ID);

    sockaddr_in addr;
    std::memset(&addr, 0, sizeof(addr));

    addr.sin_family = PF_INET;
    addr.sin_port   = portNumber.ToNetShort().GetRawValue();

    // Optionally set non-blocking.
    // Windows requires a different system call.
    if (type == Socket::ConnectionType::NonBlocking)
    {
        #ifdef _WIN32
        u_long arg = 1;
        ioctlsocket(socketId, FIONBIO, &arg);
        #else // !_WIN32
        fcntl(socketId, F_SETFL, O_NONBLOCK);
        #endif // _WIN32
    }

    // Bind server to the port and start listening at it:
    //
    if (bind(socketId, reinterpret_cast<sockaddr *>(&addr), sizeof(addr)) != 0)
    {
        Close();
        throw SocketError("Failed to 'bind()' socket to port #" +
                std::to_string(portNumber.GetRawValue()));
    }

    if (listen(socketId, pendingQueueSize) != 0)
    {
        Close();
        throw SocketError("Failed to 'listen()' at port #" +
                std::to_string(portNumber.GetRawValue()));
    }
}

SocketPtr ServerSocket::AcceptConnection() const
{
    assert(socketId != NET_INVALID_SOCKET_ID);

    errno = 0;
    const NetSocketId newSocket = accept(socketId, nullptr, nullptr);
    if (newSocket == NET_INVALID_SOCKET_ID)
    {
        // Non-blocking call / no request pending
        #ifdef _WIN32
        if (WSAGetLastError() == WSAEWOULDBLOCK)
        {
            return nullptr;
        }
        #else // !_WIN32
        if (errno == EAGAIN)
        {
            return nullptr;
        }
        #endif // _WIN32

        throw SocketError("'accept()' failed with an invalid socketId!");
    }

    return SocketPtr(new Socket(newSocket));
}

} // namespace Engine {}

These classes are part of the game "engine", hence the namespace with this name. In case anyone is curious about HostShort, it is just a thin wrapper for an u_short, mainly to avoid implicit conversions between values with different byte order. There is also a nearly identical NetShort (not shown here) that holds the value in Network Byte Order.

// Host (machine) byte order short int
class HostShort {

public:

    // Construct with the value 0.
    HostShort();

    // Can be explicitly constructed from an integer.
    // (Assumed host byte order)
    explicit HostShort(int val);

    // Can be constructed from converting a NetShort.
    explicit HostShort(const NetShort & netShort);

    // Explicit conversions:
    NetShort ToNetShort() const;
    void FromNetShort(const NetShort & netShort);

    // Raw data accessors (no byte-order conversion applied):
    u_short GetRawValue() const;
    void SetRawValue(u_short val);

private:

    u_short value;
};

// ======================================================
// HostShort Implementation:

inline HostShort::HostShort()
    : value(0)
{
}

inline HostShort::HostShort(const int val)
    : value(static_cast<u_short>(val))
{
}

inline HostShort::HostShort(const NetShort & netShort)
{
    FromNetShort(netShort);
}

inline NetShort HostShort::ToNetShort() const
{
    NetShort netShort;
    netShort.SetRawValue(htons(value));
    return netShort;
}

inline void HostShort::FromNetShort(const NetShort & netShort)
{
    value = ntohs(netShort.GetRawValue());
}

inline u_short HostShort::GetRawValue() const
{
    return value;
}

inline void HostShort::SetRawValue(const u_short val)
{
    value = val;
}

I'm not looking for any particular reviews. All suggestions, improvements and critiques are welcome.

Not enough for a review, but note that recv() may actually send less bytes than requested over a TCP socket. This is not a fatal error but you have to call recv() again with the remaining data (or use asynchronous such as poll/WSAWaitForMultipleEvents methods to "wait" for the socket to become ready again). — Martin R, Oct 29 '14 at 9:02
... of course I meant send(). recv() may receive less bytes than requested. — Martin R, Oct 29 '14 at 12:08
@MartinR - Thanks. Yeah, I'm quite unexperienced with networking, so this is being a good learning project too. I'll take a better look at the documentation and probably change that. — glampert, Oct 29 '14 at 13:34

Corbin · Accepted Answer · 2014-10-29 03:28:10Z

Looks good over all, but I have some thoughts below in order of appearance in code.

Your NetSocketId typedef should be inside of a namespace.

Your macros for invalid and erroroneous sockets could be constants.

Jut an opinion, but your sentinel comments are kind of noisy. class is clear enough for quick scanning.

// Construction / destructor: seems kind of redundant as do some of your other comments.

ClientSocketPtr and ServerSocketPtr aren't actually used in your code. This means you're making your interface bigger than it has to be, and you might regret this later if you for some reason you wanted to get of or change (incompatibly) what it was. Unless you have some kind of factory thing going on in some other code, I think letting users make the std::unique_ptr decision themselves is probably better.

SocketError::SocketError(const std::string & message)
    #ifdef _WIN32
    : std::runtime_error(message + std::string(" | sys error: ") + std::to_string(WSAGetLastError()))
    #else // !_WIN32
    : std::runtime_error(message + std::string(" | sys error: ") + std::strerror(errno))
    #endif // _WIN32
{

This makes me a bit uncomfortable. Your constructor is essentially bound to global state. The convenience of it is definitely nice, so maybe add a second constructor that allows an explicit specification of either the errno or message? (Then again, that would essentially be dead code -- hard to toe the line between pragmatism and pedantry on CR sometimes...)

You almost certainly already realize this, but just in case, your socket counting for the initialization/cleanup code is not thread-safe. That's a bit concerning since networking stuff tends to involve threads. (I have 0 knowledge of game development though, so maybe in that domain it doesn't?)

WSAStartup and WSACleanup already include counting inside of them, so it's probably simpler to just always call them in the constructor/destructor of sockets. I can't vouch for the performance implications of that, but I would imagine it's just a simple increment and decrement with exactly the same performance as your counting.

Even better, you could handle this near-implicitly by having a WSACounter class that is the first member of Socket. It's constructor and destructor would just call WSAStartup and WSACleanup, and that's all the class would do. That would make it hard to miss a call, and it would mean your constructors could fully leverage intializor lists.

Another option, if you don't care about actually releasing the winsock libraries is to have a class that does the WSAStartup/WSACleanup, and then just have a static instance of it in your implementation file.

Your comment in Socket::Socket() about socket() is on the borderline of being noise.

Socket & Socket::operator = (Socket && other) noexcept
{
    socketId       = other.socketId;
    other.socketId = NET_INVALID_SOCKET_ID;
    return *this;
}

This leaks a socket and leaves numAliveSocket incorrect if the socket in this is alive at the time of move assignment.

I might push --numAliveSockets into close() since that's when the socket actually stops being alive.

I can't decide quite how I feel about read and send being const, but I'm leaning towards that they should not be const. Imagine situations in which you have a constant socket. I would think that you would be quite surprised if that code read or wrote something.

sockaddr_in addr;
std::memset(&addr, 0, sizeof(addr));

This could be socketaddr_in addr{};

inline HostShort::HostShort(const int val)
    : value(static_cast<u_short>(val))

This seems dangerous since it's lossy. You should take a u_short, and force the user to take responsibility for constructing with either a u_short or an int that they know fits.

There is also a nearly identical NetShort (not shown here) that holds the value in Network Byte Order.

A unified type for both network byte order and host could be a little more pleasant. This could be done by having two factory functions, fromNetworkOrder and fromHostOrder that proxy through to a private constructor that knows to construct either from either one. A poorly thought out example:

class UShort {
public:
    Short fromHost(u_short val) {
        return Short(val, htons(val));
    }

    Short fromNetwork(u_short val) {
        return Short(ntohs(val), val);
    }

    u_short hostValue() const {
        return hostValue;
    }

    u_short networkValue() const {
        return networkValue;
    }

private:
    Short(u_short host, u_short network) : hostValue(host), networkValue(network)
    { }
};

As a closing note, I'm sure you've already considered it and have your own reasons against, but I'd be remiss if I didn't bring up Boost and POCO. Both have very good libraries of not just sockets, but high level networking facilities as well.

Thanks, very good suggestions. I specially like the unified UShort type. Might post a followup on that one. — glampert, Oct 29 '14 at 3:11
I wasn't aware that WSAStart/Cleanup could be called multiple times, must have missed that from the documentation. I might as well get rid of that counter. I didn't make it an atomic_int BTW because this is for a very case-specific use where I'm only creating and destroying sockets from the main thread. — glampert, Oct 29 '14 at 3:13
Socket(const NetSocketId sid) is actually explicit. And not using Boost was a mix of laziness and not wanting to add extra dependencies... But I'm considering changing that on future projects ;) — glampert, Oct 29 '14 at 3:15
@glampert Whoops, that's what I get for thinking I remembered the header with the explicit ctor :). I would assume WSAStartup/WSACleanup use thread local storage so you'd get thread safety and speed. I didn't read that far into the documentation though (speaking of which, the whole recallable thing is buried like 3/4 of the way through -- it's stupid on MS part). — Corbin, Oct 29 '14 at 3:27

Loki Astari · Answer 2 · 2014-10-29 13:00:58Z

This is a Bug:

Socket & Socket::operator = (Socket && other) noexcept
{
    socketId       = other.socketId;
    other.socketId = NET_INVALID_SOCKET_ID;
    return *this;
}

Here you should do a swap (or close the current socket before overwritting).
Because your current object may have an open socket; you want to place the current socketId into the other object so that its destructor will call close on the socket when it goes out of scope.

You should prefer the swap to a manual call to close() to make sure you move constructor stays exception safe. If you do the manual close you should also remove noexcept from the move assignment operator.

Currently you are leaking an open socket.

Init/Destroy on Zero sockets.

Do you really want to init and destroy the socket layer every time you reach zero sockets.

if (numAliveSockets == 0)
{
    common->PrintF("-------- Socket::NetInit() ---------");
}

And

if (--numAliveSockets == 0)
{
    NetShutdown();
}

The init shut-down can be quite expensive (I hear). I would rather do this once at the beginning of the program (when the first connection is created) and destroy it once at the end.

Also you are inconsistent on where the check is. On Startup your check is in NetInit() and your TearDown the check is in ~Socket() I would have expected it to be in NetShutDown() so that they look symmetrical.

I would do it like this; to make sure initialization is done only once when needed.

struct InitNetwork
{
    InitNetwork()
    {
        common->PrintF("-------- Socket::NetInit() ---------");

        PLATFOMR_SETUP_SOCKET_LAYER();   // Hide Platform specific code in macro.
    }

    ~InitNetwork()
    {
        common->PrintF("------ Socket::NetShutdown() -------");

        PLATFOMR_TEARDOWN_SOCKET_LAYER();
    }
};
void Socket::NetInit()
{
    static InitNetwork  networkInit;  // Notice the static.
}                                     // It will be created on first call
                                      // and destroyed when the application shuts down.

Blocking Vs Non Blocking Socket

Blocking sockets are not a very useful concept on a server (especially if you have the 10K problem). They have some utility on simpler clients but I still think it much more advantageous to use non blocking sockets.

Thus I see a red flag when I see this:

else if (static_cast<size_t>(result) != numBytes)
{
    throw SocketError("Failed to 'recv()' bytes! Not enough bytes read!");
}

Not receiving enough bytes should be a common situation not uncommon or exceptional.

I ran all my sockets on a single thread using libevent to detect incoming messages on all sockets (alternatives are select, pselect, epoll). When data is available I read it off. If I have a full message then I create a Job (an item of work) and go back to listening, if there is not enough of a message I hold the data in a buffer associated with the socket so I can append to it next time. Then go back to listing for data on all sockets.

You DO NOT want one thread per socket, you should be looking to handle 1000(s) of sockets per thread on the server end.

IPV4 Vs IPV6

Looks like you are locking yourself into IPV4

ClientSocket::ClientSocket(const std::string & hostNameOrIpAddress,

I would pass a structure that represents the host and connection type. Then have different types derived for IPV4 and IPV6 in that type. That way you separate out your connection code from your socket code.

class IPType
{
    public:
        virtual ~IPType() {}
        virtual int connect() = 0;
};
class IPV4: public IPType()
{
    public:
        IPV4(std::string const& hostNameOrIpAddress, int port)
        {
        }
        virtual int connect() override
        {
        }
};
// Now write a version for IPV6

ClientSocket::ClientSocket(IPType& ip)
{
     sockt = ip->connect();
}

Thanks, Loki, very relevant things. I've just got another comment about the recv/send not transferring all the data. I didn't know that. It shall be fixed. I'm a big noob when it comes to networking, so this will be a great opportunity to finally get to learn some more. I might post a followup on this. Cheers! — glampert, Oct 29 '14 at 13:42
@glampert: Before you head down rapping the socket any further. Start reading libevent (or epoll). Write something that wraps that and accepts your sockets type as input. — Loki Astari, Oct 29 '14 at 18:20

asked	10 months ago
viewed	312 times
active	10 months ago

current community

your communities

more stack exchange communities

Sockets and networking for a multi-platform game

2 Answers 2

This is a Bug:

Init/Destroy on Zero sockets.

Blocking Vs Non Blocking Socket

IPV4 Vs IPV6

Your Answer

Not the answer you're looking for? Browse other questions tagged c++ c++11 networking socket or ask your own question.

Linked

Hot Network Questions

current community

your communities

more stack exchange communities

Sockets and networking for a multi-platform game

2 Answers 2

This is a Bug:

Init/Destroy on Zero sockets.

Blocking Vs Non Blocking Socket

IPV4 Vs IPV6

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c++ c++11 networking socket or ask your own question.

Linked

Related

Hot Network Questions