Sockets and networking for a multi-platform game

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
typedef SOCKET NetSocketId;
#define NET_INVALID_SOCKET_ID INVALID_SOCKET
#define NET_SOCKET_ERROR SOCKET_ERROR
#else // !_WIN32
// Assume a Unix-like system:
#include
#include
#include
#include
#include
#include
#include
#include
typedef int NetSocketId;
#define NET_INVALID_SOCKET_ID (-1)
#define NET_SOCKET_ERROR (-1)
#endif // _WIN32

#include
#include

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 dest

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(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();
}

StackExchange Code Review Q#68240, answer score: 5