Nisse Server: Part 2 Socket Layer

Question

Getting close to a release of generic server.

Nisse Server: Part 1 Helper Functions

Here is the socket layer code. This has previously been reviewed here. But there have been some changes.

Socket.h

#ifndef THORSANVIL_SOCKET_SOCKET_H
#define THORSANVIL_SOCKET_SOCKET_H

#include <string>
#include <vector>
#include <sstream>

namespace ThorsAnvil
{
    namespace Socket
    {

// An RAII base class for handling sockets.
// Socket is movable but not copyable.
class BaseSocket
{

    int     socketId;
    protected:
        static constexpr int invalidSocketId      = -1;

        // Designed to be a base class not used used directly.
        BaseSocket(int socketId, bool blocking = false);
    public:
        int getSocketId() const {return socketId;}
    public:
        virtual ~BaseSocket();

        // Moveable but not Copyable
        BaseSocket(BaseSocket&& move)               noexcept;
        BaseSocket& operator=(BaseSocket&& move)    noexcept;
        void swap(BaseSocket& other)                noexcept;
        BaseSocket(BaseSocket const&)               = delete;
        BaseSocket& operator=(BaseSocket const&)    = delete;

        // User can manually call close
        void close();
};

// A class that can read/write to a socket
class DataSocket: public BaseSocket
{
    public:
        DataSocket(int socketId, bool blocking = false)
            : BaseSocket(socketId, blocking)
        {}

        std::pair<bool, std::size_t> getMessageData(char* buffer, std::size_t size, std::size_t alreadyGot = 0);
        std::pair<bool, std::size_t> putMessageData(char const* buffer, std::size_t size, std::size_t alreadyPut = 0);
        void        putMessageClose();
};

// A class the conects to a remote machine
// Allows read/write accesses to the remote machine
class ConnectSocket: public DataSocket
{
    public:
        ConnectSocket(std::string const& host, int port, bool blocking = false);
};

// A server socket that listens on a port for a connection
class ServerSocket: public BaseSocket
{
    static constexpr int maxConnectionBacklog = 5;
    public:
        ServerSocket(int port, bool blocking = false);

        // An accepts waits for a connection and returns a socket
        // object that can be used by the client for communication
        DataSocket accept(bool blocking = false);
};

    }
}

#endif

Socket.cpp

#include "Socket.h"
#include "Utility.h"
#include "event.h"
#include <arpa/inet.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <unistd.h>
#include <fcntl.h>
#include <sstream>
#include <stdexcept>

using namespace ThorsAnvil::Socket;
#pragma vera_pushoff
using SocketAddr    = struct sockaddr;
using SocketStorage = struct sockaddr_storage;
using SocketAddrIn  = struct sockaddr_in;
#pragma vera_pop

BaseSocket::BaseSocket(int socketId, bool blocking)
    : socketId(socketId)
{
    if (socketId == -1)
    {
        throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__, ": bad socket: ", systemErrorMessage()));
    }
    if (!blocking && evutil_make_socket_nonblocking(socketId) != 0)
    {
        throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__, ": evutil_make_socket_nonblocking: failed to make non blocking: "));
    }
}

BaseSocket::~BaseSocket()
{
    if (socketId == invalidSocketId)
    {
        // This object has been closed or moved.
        // So we don't need to call close.
        return;
    }

    try
    {
        close();
    }
    catch (...)
    {
        // We should log this
        // TODO: LOGGING CODE HERE

        // If the user really want to catch close errors
        // they should call close() manually and handle
        // any generated exceptions. By using the
        // destructor they are indicating that failures is
        // an OK condition.
    }
}

void BaseSocket::close()
{
    return;
    if (socketId == invalidSocketId)
    {
        throw std::logic_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__, ": accept called on a bad socket object (this object was moved)"));
    }
    while (true)
    {
        int state = ::close(socketId);
        if (state == invalidSocketId)
        {
            break;
        }
        switch (errno)
        {
            case EBADF: throw std::domain_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__, ": close: ", socketId, " ", systemErrorMessage()));
            case EIO:   throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__, ": close: ", socketId, " ", systemErrorMessage()));
            case EINTR:
            {
                        // TODO: Check for user interrupt flags.
                        //       Beyond the scope of this project
                        //       so continue normal operations.
                break;
            }
            default:    throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__, ": close: ", socketId, " ", systemErrorMessage()));
        }
    }
    socketId = invalidSocketId;
}

void BaseSocket::swap(BaseSocket& other) noexcept
{
    using std::swap;
    swap(socketId,   other.socketId);
}

BaseSocket::BaseSocket(BaseSocket&& move) noexcept
    : socketId(invalidSocketId)
{
    move.swap(*this);
}

BaseSocket& BaseSocket::operator=(BaseSocket&& move) noexcept
{
    move.swap(*this);
    return *this;
}

ConnectSocket::ConnectSocket(std::string const& host, int port, bool blocking)
    : DataSocket(::socket(PF_INET, SOCK_STREAM, 0), blocking)
{
    SocketAddrIn serverAddr{};
    serverAddr.sin_family       = AF_INET;
    serverAddr.sin_port         = htons(port);
    serverAddr.sin_addr.s_addr  = inet_addr(host.c_str());

    if (::connect(getSocketId(), reinterpret_cast<SocketAddr*>(&serverAddr), sizeof(serverAddr)) != 0)
    {
        close();
        throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::ConnectSocket::", __func__, ": connect: ", systemErrorMessage()));
    }
}

ServerSocket::ServerSocket(int port, bool blocking)
    : BaseSocket(::socket(PF_INET, SOCK_STREAM, 0), blocking)
{
    SocketAddrIn    serverAddr = {};
    serverAddr.sin_family       = AF_INET;
    serverAddr.sin_port         = htons(port);
    serverAddr.sin_addr.s_addr  = INADDR_ANY;

    if (::bind(getSocketId(), reinterpret_cast<SocketAddr*>(&serverAddr), sizeof(serverAddr)) != 0)
    {
        close();
        throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::ServerSocket::", __func__, ": bind: ", systemErrorMessage()));
    }

    if (::listen(getSocketId(), maxConnectionBacklog) != 0)
    {
        close();
        throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::ServerSocket::", __func__, ": listen: ", systemErrorMessage()));
    }
}

DataSocket ServerSocket::accept(bool blocking)
{
    if (getSocketId() == invalidSocketId)
    {
        throw std::logic_error(buildErrorMessage("ThorsAnvil::Socket::ServerSocket::", __func__, ": accept called on a bad socket object (this object was moved)"));
    }

    SocketStorage    serverStorage;
    socklen_t        addr_size   = sizeof serverStorage;
    int newSocket = ::accept(getSocketId(), reinterpret_cast<SocketAddr*>(&serverStorage), &addr_size);
    if (newSocket == -1)
    {
        throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::ServerSocket:", __func__, ": accept: ", systemErrorMessage()));
    }
    return DataSocket(newSocket, blocking);
}

std::pair<bool, std::size_t> DataSocket::getMessageData(char* buffer, std::size_t size, std::size_t alreadyGot)
{
    if (getSocketId() == 0)
    {
        throw std::logic_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": accept called on a bad socket object (this object was moved)"));
    }

    std::size_t     dataRead  = alreadyGot;
    while (dataRead < size)
    {
        // The inner loop handles interactions with the socket.
        std::size_t get = ::read(getSocketId(), buffer + dataRead, size - dataRead);
        if (get == static_cast<std::size_t>(-1))
        {
            switch (errno)
            {
                case EBADF:
                case EFAULT:
                case EINVAL:
                case ENXIO:
                {
                    // Fatal error. Programming bug
                    throw std::domain_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": read: critical error: ", systemErrorMessage()));
                }
                case EIO:
                case ENOBUFS:
                case ENOMEM:
                {
                   // Resource acquisition failure or device error
                    throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": read: resource failure: ", systemErrorMessage()));
                }
                case EINTR:
                {
                    // TODO: Check for user interrupt flags.
                    //       Beyond the scope of this project
                    //       so continue normal operations.
                    continue;
                }
                case ETIMEDOUT:
                case EAGAIN:
                //case EWOULDBLOCK:
                {
                    // Temporary error.
                    // Simply retry the read.
                    return {true, dataRead};
                }
                case ECONNRESET:
                case ENOTCONN:
                {
                    // Connection broken.
                    // Return the data we have available and exit
                    // as if the connection was closed correctly.
                    return {false, dataRead};
                }
                default:
                {
                    throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": read: returned -1: ", systemErrorMessage()));
                }
            }
        }
        if (get == 0)
        {
            return {false, dataRead};
        }
        dataRead += get;
    }

    return {true, dataRead};
}

std::pair<bool, std::size_t> DataSocket::putMessageData(char const* buffer, std::size_t size, std::size_t alreadyPut)
{
    std::size_t     dataWritten = alreadyPut;

    while (dataWritten < size)
    {
        std::size_t put = ::write(getSocketId(), buffer + dataWritten, size - dataWritten);
        if (put == static_cast<std::size_t>(-1))
        {
            switch (errno)
            {
                case EINVAL:
                case EBADF:
                case ECONNRESET:
                case ENXIO:
                case EPIPE:
                {
                    // Fatal error. Programming bug
                    throw std::domain_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": write: critical error: ", systemErrorMessage()));
                }
                case EDQUOT:
                case EFBIG:
                case EIO:
                case ENETDOWN:
                case ENETUNREACH:
                case ENOSPC:
                {
                    // Resource acquisition failure or device error
                    throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": write: resource failure: ", systemErrorMessage()));
                }
                case EINTR:
                {
                    // TODO: Check for user interrupt flags.
                    //       Beyond the scope of this project
                    //       so continue normal operations.
                    continue;
                }
                case ETIMEDOUT:
                case EAGAIN:
                //case EWOULDBLOCK:
                {
                    // Temporary error.
                    // Simply retry the read.
                    return {true, dataWritten};
                }
                default:
                {
                    throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": write: returned -1: ", systemErrorMessage()));
                }
            }
        }
        dataWritten += put;
    }
    return {true, dataWritten};
}

void DataSocket::putMessageClose()
{
    if (::shutdown(getSocketId(), SHUT_WR) != 0)
    {
        throw std::domain_error(buildErrorMessage("ThorsAnvil::Socket::DataSocket::", __func__, ": shutdown: critical error: ", systemErrorMessage()));
    }
}

Answer 1

1. In-class-initialization of socketId to invalidSocketId would simplify things.

2. I would move much of the implementation of BaseSocket into the header as it's simple.

3. There's no reason not to have a constexpr noexcept default-ctor for BaseSocket.

4. Your use of libevent for abstracting the different handling of sockets on Windows and Unixoids is very intermittent. As an example, you use evutil_make_socket_nonblocking() but neither evutil_closesocket() nor evutil_socket_t.

5. Sometimes, you use your own custom constant invalidSocketId, sometimes -1. I couldn't find any use of INVALID_HANDLE_VALUE.

6. 166 is a bit long for a line. And there's no need, plenty of options for wrapping them to something sane. Remember that horizontal scrolling kills readability.

7. I would avoid spamming spaces trying to vertically align code-elements. Visually locking independent lines together makes changes cascade to non-modified lines, or leads to curious artifacts when not spreading them on every modification. There are such relics in your code...

8. Well, I don't think there's much to say about the TODO markers left.

9. BaseSocket::close() is very buggy:

   • As vnp remarked, it starts with a spurious return;, making it a no-op.
   • The while(true)-loop is useless.
   • On top of that, it returns zero on success, -1 on failure.

10. I wonder why you make maxConnectionBacklog a constant instead of a (defaulted) ctor-argument for ServerSocket.

11. You know passing nullptr and 0 as the last two arguments to ::accept is acceptable?

12. DataSocket::getMessageData() assumes 0 is an invalid socket handle. You know that's wrong.

13. There's no reason to proactively check whether the socket handle is valid. Just let the call to read() fail, as you did for write().

Answer 2

I added unit tests to my code above (after adding most of @Deduplicator suggestions. This discovered a few more issues:

The close() can fail leaving the object in an invalid state (Especially after fixing that spurious return).

void BaseSocket::close()
{
    // If you call close on a socket that is invalid then
    // there is a bug in the calling code (i.e. not my code).
    //
    // This means you have either previously closed the object
    // or have moved the socket to another object. Either is
    // a bad logic_error in the calling code.
    if (socketId == invalidSocketId)
    {
        throw std::logic_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__,
                                                 ": accept called on a bad socket object (this object was moved)"));
    }

    // Need a loop.
    // ::close may fail with EINTR which mean we  need to try again.
    while (true)
    {
        if (::close(socketId) == -1)
        {
            switch (errno)
            {
                // If we throw an exception this object is no longer
                // valid so set the state of the socket appropriately.
                // That way the destructor will not try close again.
                case EBADF: socketId = invalidSocketId;
                            throw std::domain_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__,
                                                                      ": close: ", socketId, " ", systemErrorMessage()));
                case EIO:   socketId = invalidSocketId;
                            throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__,
                                                                       ": close: ", socketId, " ", systemErrorMessage()));
                case EINTR:
                {
                    // TODO: Check for user interrupt flags.
                    //       Beyond the scope of this project
                    //       so continue normal operations.

                    // This was previously a break.
                    // It should be a continue.
                    continue;
                }
                default:    socketId = invalidSocketId;
                            throw std::runtime_error(buildErrorMessage("ThorsAnvil::Socket::BaseSocket::", __func__,
                                                                       ": close: ", socketId, " ", systemErrorMessage()));
            }
        }
        break;
    }
    socketId = invalidSocketId;
}

Geting the hostaddress does not work as expected:

 serverAddr.sin_addr.s_addr  = inet_addr(host.c_str());

The call inet_addr() converts a string in IP format into an address. But this is not really that useful. I needed a function that converts a hostname or IP address string into an address.

The ConnectSocket constructor allowed you to specific a non blocking connection (and it was the default). In reality that will not work for a connect call (as you are establishing the connection).

So I removed the option to making it nonblocking and it always passes true down to the base class.