3
\$\begingroup\$

Summary

This is my core networking code for the lockstep RTS game that I am creating. Clients connect to a relay server via a TCP socket, and any packets sent to the relay server are forwarded to all other players.

The relay server does not care about the packet contents or game state, it just passes packets around. Clients send a packet every tick containing their commands, and a tick is only processed once clients have received the commands of all other players - this ensures that everyone is running an identical simulation, so all clients remain in-sync. To mitigate lag, commands are scheduled for 'n' ticks in the future (currently set to 10 ticks, or ~166ms).

Currently this seems to be working well (at least on Windows).

Any feedback is welcome, be it about the code, the architecture, or any other decisions I have made. I am always looking to improve!

Socket class

The idea is for this to be a cross-platform wrapper around a raw socket that uses RAII to manage the connection.

  • Named factory methods are provided to initialize the socket for a client/server.
  • The send method sends the given buffer, and the receive method blocks until it has filled the given buffer.
  • If an error occurs during initialization, an exception is thrown (I know exceptions aren't popular in game development, but I am using them nonetheless).
  • If an error occurs while sending/receiving data, the socket is closed gracefully.
  • A Socket can be moved, but not copied.

Socket.h

This contains the class declaration, and should (ideally) be completely platform-agnostic.

#pragma once

#include <cstdint>
#include <memory>
#include <string>
#include <vector>

// Ideally we would not have any platform-specific stuff here,
// but this seems unavoidable since we need to know the underlying type!
#ifdef _WIN32
#include <winsock2.h>
#else
// I am only targeting Windows for now, so don't worry too much about this.
typedef int SOCKET;
#endif

namespace Rival {

enum class SocketState : std::uint8_t
{
    Open,
    Closed
};

/**
 * Wrapper class for a native socket.
 *
 * Implementation details are platform-specific.
 */
class Socket
{
public:
    /** Creates a listening socket connected to localhost. */
    static Socket createServer(int port);

    /** Creates a socket and attempts to connect it to the given address and port. */
    static Socket createClient(const std::string& address, int port);

    /** Creates a socket that wraps a raw socket handle. The socket is assumed to be open if the handle is valid. */
    static Socket wrap(SOCKET rawSocket);

    ~Socket();

    bool Socket::operator==(const Socket& other) const;
    bool Socket::operator!=(const Socket& other) const;

    // Allow moving but prevent copying and move-assignment
    Socket(const Socket& other) = delete;
    Socket(Socket&& other) noexcept;
    Socket& operator=(const Socket& other) = delete;
    Socket& operator=(Socket&& other) = delete;

    /** Blocking call that waits for a new connection. */
    Socket accept();

    /** Closes the socket; forces any blocking calls to return. */
    void close() noexcept;

    /** Determines if this socket is valid. */
    bool isValid() const;

    /** Determines if this socket is closed. */
    bool isClosed() const;

    /** Sends data on this socket. */
    void send(std::vector<char>& buffer);

    /** Blocking call that waits for data to arrive and adds it to the given buffer. */
    void receive(std::vector<char>& buffer);

private:
    Socket(const std::string& address, int port, bool server);
    Socket(SOCKET rawSocket);

    void init();

private:
    SOCKET sock;
    SocketState state = SocketState::Closed;
};

}  // namespace Rival

Socket.cpp

This contains any socket functionality that is common to all platforms.

#include "pch.h"

#include "net/Socket.h"

namespace Rival {

Socket Socket::createServer(int port)
{
    return { "localhost", port, true };
}

Socket Socket::createClient(const std::string& address, int port)
{
    return { address, port, false };
}

Socket Socket::wrap(SOCKET rawSocket)
{
    return { rawSocket };
}

Socket::Socket(SOCKET rawSocket)
    : sock(rawSocket)
{
    init();

    state = (rawSocket == INVALID_SOCKET) ? SocketState::Closed : SocketState::Open;
}

Socket::~Socket()
{
    close();
}

bool Socket::operator==(const Socket& other) const
{
    return sock == other.sock;
}

bool Socket::operator!=(const Socket& other) const
{
    return !(*this == other);
}

bool Socket::isClosed() const
{
    return state == SocketState::Closed;
}

}  // namespace Rival

WindowsSocket.cpp

This common the Windows-specific socket functionality.

#include "pch.h"

#ifdef _WIN32

// These comments...
#include "net/Socket.h"
// ... prevent the auto-formatter from moving the include

#include <winsock2.h>

#include <ws2tcpip.h>

#include <iostream>
#include <stdexcept>
#include <utility>  // std::exchange

namespace Rival {

Socket::Socket(const std::string& address, int port, bool server)
{
    // Specify socket properties
    addrinfo hints;
    ZeroMemory(&hints, sizeof(hints));
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;
    if (server)
    {
        hints.ai_flags = AI_PASSIVE;
    }

    // Resolve the local address and port to be used by the server
    addrinfo* addrInfo = nullptr;
    if (int err = getaddrinfo(address.c_str(), std::to_string(port).c_str(), &hints, &addrInfo))
    {
        throw std::runtime_error("Failed to get net address: " + std::to_string(err));
    }

    // Create the socket
    sock = INVALID_SOCKET;
    sock = socket(addrInfo->ai_family, addrInfo->ai_socktype, addrInfo->ai_protocol);

    // Check for errors
    if (sock == INVALID_SOCKET)
    {
        int err = WSAGetLastError();
        freeaddrinfo(addrInfo);
        throw std::runtime_error("Failed to create socket: " + std::to_string(err));
    }

    // Common socket initialization
    init();

    if (server)
    {
        // Bind the socket
        int bindResult = bind(sock, addrInfo->ai_addr, static_cast<int>(addrInfo->ai_addrlen));
        if (bindResult == SOCKET_ERROR)
        {
            int err = WSAGetLastError();
            freeaddrinfo(addrInfo);
            throw std::runtime_error("Failed to bind socket: " + std::to_string(err));
        }

        // Address info is no longer needed
        freeaddrinfo(addrInfo);

        // Listen (server-only)
        if (listen(sock, SOMAXCONN) == SOCKET_ERROR)
        {
            throw std::runtime_error("Failed to listen on socket: " + std::to_string(WSAGetLastError()));
        }
    }
    else
    {
        // Connect to server
        int connectResult = connect(sock, addrInfo->ai_addr, static_cast<int>(addrInfo->ai_addrlen));
        int err = WSAGetLastError();
        if (connectResult == SOCKET_ERROR)
        {
            sock = INVALID_SOCKET;
        }

        // Address info is no longer needed
        freeaddrinfo(addrInfo);

        if (sock == INVALID_SOCKET)
        {
            throw std::runtime_error("Failed to connect to server: " + std::to_string(err));
        }
    }

    if (isValid())
    {
        state = SocketState::Open;
    }
}

Socket::Socket(Socket&& other) noexcept
    // Reset the source object so its destructor is harmless
    : sock(std::exchange(other.sock, INVALID_SOCKET))
    , state(std::exchange(other.state, SocketState::Closed))
{
}

void Socket::init()
{
    // Disable Nagle algorithm to ensure packets are not held up
    BOOL socketOptionValue = TRUE;
    setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, (char*) (&socketOptionValue), sizeof(BOOL));
}

void Socket::close() noexcept
{
    if (state == SocketState::Closed)
    {
        return;
    }

    state = SocketState::Closed;
    closesocket(sock);
}

Socket Socket::accept()
{
    SOCKET clientSocket = INVALID_SOCKET;

    clientSocket = ::accept(sock, nullptr, nullptr);

    if (clientSocket == INVALID_SOCKET && !isClosed())
    {
        std::cout << "Failed to accept client: " + std::to_string(WSAGetLastError()) << "\n";
    }

    return Socket::wrap(clientSocket);
}

bool Socket::isValid() const
{
    return sock != INVALID_SOCKET;
}

void Socket::send(std::vector<char>& buffer)
{
    std::size_t bytesSent = 0;

    while (bytesSent < buffer.size())
    {
        int bytesRemaining = buffer.size() - bytesSent;
        int result = ::send(sock, buffer.data() + bytesSent, bytesRemaining, 0);

        if (result == SOCKET_ERROR)
        {
            if (isClosed())
            {
                // Socket has been closed, so just abort the send
                break;
            }
            else
            {
                // Socket is still open on our side but may have been closed by the other side
                std::cerr << "Failed to send on socket: " + std::to_string(WSAGetLastError()) << "\n";
                close();
                break;
            }
        }

        bytesSent += result;
    }
}

void Socket::receive(std::vector<char>& buffer)
{
    std::size_t bytesReceived = 0;

    while (bytesReceived < buffer.size())
    {
        int bytesExpected = buffer.size() - bytesReceived;
        int result = ::recv(sock, buffer.data() + bytesReceived, bytesExpected, 0);

        if (result == SOCKET_ERROR)
        {
            if (isClosed())
            {
                // Socket has been closed, so just abort the read
                break;
            }
            else
            {
                // Socket is still open on our side but may have been closed by the other side
                std::cerr << "Failed to read from socket: " + std::to_string(WSAGetLastError()) << "\n";
                close();
                break;
            }
        }

        if (result == 0)
        {
            // Connection has been gracefully closed
            close();
            break;
        }

        bytesReceived += result;
    }
}

}  // namespace Rival

#endif

Reading and relaying packets

Without getting too bogged down in the details of the Connection class, here is the code that receives data from the socket.

On the client:

  • Received data is parsed into packets by a PacketFactory.
  • Packets are queued until getReceivedPackets is called.

On the server:

  • Received packets are wrapped in a RelayedPacket.
  • These are passed to a listener class for forwarding.
void Connection::receiveThreadLoop()
{
    while (!socket.isClosed())
    {
        // First read the packet size
        if (!readFromSocket(Packet::sizeBytes))
        {
            break;
        }

        // Extract the packet size from the buffer
        std::size_t offset = 0;
        int nextPacketSize = 0;
        BufferUtils::readFromBuffer(recvBuffer, offset, nextPacketSize);
        recvBuffer.clear();

        // Sanity-check the packet size
        if (nextPacketSize > maxBufferSize)
        {
            throw std::runtime_error("Unexpected packet size: " + std::to_string(nextPacketSize));
        }

        // Now read the packet itself
        if (!readFromSocket(nextPacketSize))
        {
            break;
        }

        // If this Connection has no packet factory, then it belongs to the relay server. The relay server doesn't care
        // about the contents of incoming packets, it just wraps them in RelayedPackets.
        std::shared_ptr<const Packet> packet = packetFactory
                ? packetFactory->deserialize(recvBuffer)
                : std::make_shared<RelayedPacket>(recvBuffer, remoteClientId);

        if (packet)
        {
            // Pass packets directly to the listener if present, otherwise queue them until requested
            if (listener)
            {
                listener->onPacketReceived(*this, packet);
            }
            else
            {
                std::scoped_lock lock(receivedPacketsMutex);
                receivedPackets.push_back(packet);
            }
        }
    }
}

bool Connection::readFromSocket(std::size_t numBytes)
{
    recvBuffer.resize(numBytes);
    socket.receive(recvBuffer);

    // The socket may get closed during a call to `receive`
    bool success = !socket.isClosed();
    return success;
}

std::vector<std::shared_ptr<const Packet>> Connection::getReceivedPackets()
{
    std::vector<std::shared_ptr<const Packet>> packetsToReturn;

    {
        std::scoped_lock lock(receivedPacketsMutex);
        packetsToReturn = receivedPackets;
        receivedPackets.clear();
    }

    return packetsToReturn;
}

BufferUtils

Finally, here are a couple of utility methods that I use to add data to / extract data from a buffer:

#pragma once

#include <cstddef>  // std::size_t
#include <cstring>  // std::memcpy
#include <stdexcept>
#include <vector>

namespace Rival { namespace BufferUtils {

/** Adds a value to the end of the given buffer.
 * This should not be used for anything which manages its own memory (e.g. containers and strings). */
template <typename T>
void addToBuffer(std::vector<char>& buffer, const T& val)
{
    // Later, we may need to ensure a certain endianness for cross-platform compatibility.
    // See: https://stackoverflow.com/questions/544928/reading-integer-size-bytes-from-a-char-array

    size_t requiredBufferSize = buffer.size() + sizeof(val);
    if (requiredBufferSize > buffer.capacity())
    {
        throw std::runtime_error("Trying to overfill buffer");
    }

    char* destPtr = buffer.data() + buffer.size();

    // Since we are writing to the vector's internal memory we need to manually change the size
    buffer.resize(requiredBufferSize);

    std::memcpy(destPtr, &val, sizeof(val));
}

/** Reads a value from the given buffer, at some offset.
 * The value is stored in dest, and the offset is increased by the size of the value. */
template <typename T>
void readFromBuffer(const std::vector<char>& buffer, std::size_t& offset, T& dest)
{
    if (offset + sizeof(dest) > buffer.size())
    {
        throw std::runtime_error("Trying to read past end of buffer");
    }

    std::memcpy(&dest, buffer.data() + offset, sizeof(dest));
    offset += sizeof(dest);
}

}}  // namespace Rival::BufferUtils
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

A quick look at your Socket class:


Use INVALID_SOCKET to indicate a closed socket

SOCKET sock;
SocketState state = SocketState::Closed;

If you use INVALID_SOCKET to indicate a closed socket, there's no need for the state variable. Using two variables is potentially error-prone, since they can get out of sync.


Constructors / assignment

Socket(const std::string& address, int port, bool server);
Socket(SOCKET rawSocket);

I think it's fine to make these public.

Socket(Socket&& other) noexcept;
Socket& operator=(Socket&& other) = delete;

Not sure why we prevent move assignment. It could be useful and is easy to implement. Especially if we add a default constructor that is simply a closed socket.


A single isOpen() function

/** Determines if this socket is valid. */
bool isValid() const;

/** Determines if this socket is closed. */
bool isClosed() const;

If we use INVALID_SOCKET as mentioned above, we only need one of these (I'd probably call it isOpen).


Const-correctness

void send(std::vector<char>& buffer);

Since we don't want to alter buffer, we should pass it by const&, not just &.


Refactoring the Socket constructor

Socket::Socket(const std::string& address, int port, bool server)
{
    // Specify socket properties
    addrinfo hints;
    ZeroMemory(&hints, sizeof(hints));
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;
    if (server)
    {
        hints.ai_flags = AI_PASSIVE;
    }

    // Resolve the local address and port to be used by the server
    addrinfo* addrInfo = nullptr;
    if (int err = getaddrinfo(address.c_str(), std::to_string(port).c_str(), &hints, &addrInfo))
    {
        throw std::runtime_error("Failed to get net address: " + std::to_string(err));
    }

    // Create the socket
    sock = INVALID_SOCKET;
    sock = socket(addrInfo->ai_family, addrInfo->ai_socktype, addrInfo->ai_protocol);

    // Check for errors
    if (sock == INVALID_SOCKET)
    {
        int err = WSAGetLastError();
        freeaddrinfo(addrInfo);
        throw std::runtime_error("Failed to create socket: " + std::to_string(err));
    }

    // Common socket initialization
    init();

    if (server)
    {
        // Bind the socket
        int bindResult = bind(sock, addrInfo->ai_addr, static_cast<int>(addrInfo->ai_addrlen));
        if (bindResult == SOCKET_ERROR)
        {
            int err = WSAGetLastError();
            freeaddrinfo(addrInfo);
            throw std::runtime_error("Failed to bind socket: " + std::to_string(err));
        }

        // Address info is no longer needed
        freeaddrinfo(addrInfo);

        // Listen (server-only)
        if (listen(sock, SOMAXCONN) == SOCKET_ERROR)
        {
            throw std::runtime_error("Failed to listen on socket: " + std::to_string(WSAGetLastError()));
        }
    }
    else
    {
        // Connect to server
        int connectResult = connect(sock, addrInfo->ai_addr, static_cast<int>(addrInfo->ai_addrlen));
        int err = WSAGetLastError();
        if (connectResult == SOCKET_ERROR)
        {
            sock = INVALID_SOCKET;
        }

        // Address info is no longer needed
        freeaddrinfo(addrInfo);

        if (sock == INVALID_SOCKET)
        {
            throw std::runtime_error("Failed to connect to server: " + std::to_string(err));
        }
    }

    if (isValid())
    {
        state = SocketState::Open;
    }
}

This is a long function, and needs to be split up into separate, reusable parts. If we can make the first two parts (address lookup and opening the socket) less burdensome, we can avoid using the boolean server flag, which is a bit of an anti-pattern, and just write two separate functions.

C++ is awkward for handling errors (either returning an error code, or throwing), which can make the code more long-winded. Consider using a std::system_error& parameter as an output parameter.

One other thing that can help here is wrapping the address info pointer from getaddrinfo in a unique_ptr with a custom deleter. This removes the burden of calling freeaddrinfo manually.

So... you might end up with something very roughly like this:

Socket::Socket():
    m_socket(INVALID_SOCKET) { }
    
Socket::Socket(SOCKET handle):
    m_socket(handle) { }

Socket::Socket(int domain, int type, int protocol, std::system_error& ec):
    m_socket(::socket(domain, type, protocol))
{
    if (!is_open())
        ec = std::system_error(std::error_code(::WSAGetLastError(), std::system_category()));
}

Socket::~Socket()
{
    auto ec = std::system_error();
    (void)close(ec);
}

void Socket::is_open() const
{
    return m_socket != INVALID_SOCKET;
}

bool Socket::close(std::system_error& ec)
{
    if (!is_open())
        return;
    
    auto const result = ::closesocket(m_socket);
    
    if (result != 0)
    {
        ec = std::system_error(std::error_code(::WSAGetLastError(), std::system_category()));
        return false;
    }
    
    return true;
}

bool Socket::bind(sockaddr const* address, socklen_t address_length, std::system_error& ec)
{
    auto const result = ::bind(m_socket, address, address_length);
    
    if (result != 0)
    {
        ec = std::system_error(std::error_code(::WSAGetLastError(), std::system_category())); // TODO: abstract into a get_last_error()!
        return false;
    }
    
    return true;
}

// listen, connect, etc. left as an exercise for the reader.

...

using addrinfo_ptr = std::unique_ptr<::addrinfo, void(*)(::addrinfo*)>;

addrinfo_ptr lookup_address(char const* node, char const* service, int domain, int type, int protocol, int flags, std::system_error& ec)
{
    assert(node || service);
    
    auto hints = ::addrinfo();
    std::memset(&hints, 0, sizeof(::addrinfo));
    hints.ai_family = domain;
    hints.ai_socktype = type;
    hints.ai_protocol = protocol;
    hints.ai_flags = flags;
    
    auto out = (::addrinfo*)nullptr;
    auto const result = ::getaddrinfo(node, service, &hints, &out);
    
    if (result != 0)
    {
        ec = std::system_error(std::error_code(result, std::system_category())); // note: not WSAGetLastError
        return { };
    }
    
    assert(out);
    
    return addrinfo_ptr(out, &::freeaddrinfo);
}

addrinfo_ptr get_local_address_info(int domain, int type, int protocol, std::uint16_t port, std::system_error& ec)
{
    auto const port_str = std::to_string(port);
    int flags = AI_PASSIVE | AI_NUMERICSERV;
    return lookup_address(nullptr, port_str.data(), domain, type, protocol, flags, ec);
}

addrinfo_ptr get_address_info(int domain, int type, int protocol, std::string const& node, std::uint16_t port, std::system_error& ec)
{
    auto const port_str = std::to_string(port);
    int flags = AI_NUMERICSERV;
    return lookup_address(node.data(), port_str.data(), domain, type, protocol, flags, ec);
}

...

Socket create_server(std::uint16_t port)
{
    auto const domain = AF_INET;
    auto const type = SOCK_STREAM;
    auto const protocol = IPPROTO_TCP;
    
    auto ec = std::system_error();
    auto const addrinfoptr = get_local_address_info(domain, type, protocol, port, ec);
    
    if (!addrinfoptr)
        throw std::runtime_error(std::string("Address lookup failed: ") + ec.what());
    
    auto socket = Socket(domain, type, protocol, ec);
    
    if (!socket.is_open())
        throw std::runtime_error(std::string("Open socket failed: ") + ec.what());
    
    if (!socket.bind(addrinfoptr->ai_addr, addrinfoptr->ai_addrlen, ec))
        throw std::runtime_error(std::string("Bind socket failed: ") + ec.what());
    
    if (!socket.listen(SOMAXCONN, ec))
        throw std::runtime_error(std::string("Listen failed: ") + ec.what());
    
    return socket;
}

Socket create_client(std::string const& host, std::uint16_t port)
{
    auto const domain = AF_INET;
    auto const type = SOCK_STREAM;
    auto const protocol = IPPROTO_TCP;
    
    auto ec = std::system_error();
    auto const addrinfoptr = get_address_info(domain, type, protocol, host, port, ec);
    
    if (!addrinfoptr)
        throw std::runtime_error(std::string("Address lookup failed: ") + ec.what());
    
    auto socket = Socket(domain, type, protocol, ec);
    
    if (!socket.is_open())
        throw std::runtime_error(std::string("Open socket failed: ") + ec.what());
    
    if (!socket.connect(addrinfoptr->ai_addr, addrinfoptr->ai_addrlen, ec))
        throw std::runtime_error(std::string("Connect failed: ") + ec.what());
    
    return socket;
}

(Note: not compiled / tested)

There are other patterns you could use for dealing with return values / error handling. These include throwing errors (and the more burdensome catching), or using something like std::expected.

Things get a little more complicated if you want to handle non-blocking sockets. It just means checking a couple of "error" values (e.g. WSAEWOULDBLOCK) inside our hypothetical Socket::connect() and treating them as success instead.


Use std::size_t

    int bytesRemaining = buffer.size() - bytesSent;
    ...
    int bytesExpected = buffer.size() - bytesReceived;

These guys should be std::size_t.


A fixed-size buffer?

void Socket::receive(std::vector<char>& buffer)
{
    std::size_t bytesReceived = 0;

    while (bytesReceived < buffer.size())
    {
        int bytesExpected = buffer.size() - bytesReceived;
        int result = ::recv(sock, buffer.data() + bytesReceived, bytesExpected, 0);

        ...

        bytesReceived += result;
    }
}

Looping until we receive all the bytes in a fixed size buffer is very limiting and error-prone. I think this might be better enforced outside the Socket class. It's quite possible we'd want to receive a packet of unknown size in future.

(And the same with sending).

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you for the detailed review! Some great points there, I will take them on board. Regarding the fixed-size buffer, I always send (and read) the packet size first so I know exactly how much data to expect. At least for my use case, I don't expect any packets of an unknown size, although I appreciate that this approach may be problematic in other situations. \$\endgroup\$
    – Dan
    May 2, 2023 at 18:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.