C++ Socket Part-2

Question

In my ongoing attempts to become a better blog writer I have some written some more code that needs reviewing.

Full Source.
First Article.

This is a simple C++ wrapper for Sockets. Here the bit I am not 100% happy with is needing to introduce a Socket.tpp file to hold getMessageData(). This is because the function is templated with a callback function that is used to decide if we have read enough. This is not used by the ProtocolSimple (in Version 2) but will be used by ProtocolHTTP (in Version 3).

##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:
        // Designed to be a base class not used used directly.
        BaseSocket(int socketId);
        int getSocketId() const {return socketId;}
    public:
        ~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)
            : BaseSocket(socketId)
        {}

        template<typename F>
        std::size_t getMessageData(char* buffer, std::size_t size, F scanForEnd = [](std::size_t){return false;});
        void        putMessageData(char const* buffer, std::size_t size);
        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);
};
/ A server socket that listens on a port for a connection
class ServerSocket: public BaseSocket
{
    public:
        ServerSocket(int port);

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

    }
}

#include "Socket.tpp"

#endif

##Socket.tpp

#include "Utility.h"
#include <stdexcept>
#include <sys/types.h>
#include <sys/uio.h>
#include <unistd.h>

namespace ThorsAnvil
{
    namespace Socket
    {

template<typename F>
std::size_t DataSocket::getMessageData(char* buffer, std::size_t size, F scanForEnd)
{
    if (getSocketId() == 0)
    {
        throw std::logic_error(buildErrorMessage("DataSocket::getMessage: accept called on a bad socket object (this object was moved)"));
    }

    std::size_t     dataRead  = 0;
    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("DataSocket::getMessageDataFromStream: read: critical error: ", strerror(errno)));
                }
                case EIO:
                case ENOBUFS:
                case ENOMEM:
                {
                   // Resource acquisition failure or device error
                    throw std::runtime_error(buildErrorMessage("DataSocket::getMessageDataFromStream: read: resource failure: ", strerror(errno)));
                }
                case EINTR:
                    // TODO: Check for user interrupt flags.
                    //       Beyond the scope of this project
                    //       so continue normal operations.
                case ETIMEDOUT:
                case EAGAIN:
                {
                    // Temporary error.
                    // Simply retry the read.
                    continue;
                }
                case ECONNRESET:
                case ENOTCONN:
                {
                    // Connection broken.
                    // Return the data we have available and exit
                    // as if the connection was closed correctly.
                    get = 0;
                    break;
                }
                default:
                {
                    throw std::runtime_error(buildErrorMessage("DataSocket::getMessageDataFromStream: read: returned -1: ", strerror(errno)));
                }
            }
        }
        if (get == 0)
        {
            break;
        }
        dataRead += get;
        if (scanForEnd(dataRead))
        {
            break;
        }
    }

    return dataRead;
}

    }
}

##Socket.cpp

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

#include <iostream>

using namespace ThorsAnvil::Socket;

BaseSocket::BaseSocket(int socketId)
    : socketId(socketId)
{
    if (socketId == -1)
    {
        throw std::runtime_error(buildErrorMessage("BaseSocket::BaseSocket: bad socket: ", strerror(errno)));
    }
}

BaseSocket::~BaseSocket()
{
    if (socketId == 0)
    {
        // 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()
{
    if (socketId == 0)
    {
        throw std::logic_error(buildErrorMessage("DataSocket::close: accept called on a bad socket object (this object was moved)"));
    }
    while(true)
    {
        int state = ::close(socketId);
        if (state == 0)
        {
            break;
        }
        switch(errno)
        {
            case EBADF: throw std::domain_error(buildErrorMessage("BaseSocket::close: close: EBADF: ", socketId, " ", strerror(errno)));
            case EIO:   throw std::runtime_error(buildErrorMessage("BaseSocket::close: close: EIO:  ", socketId, " ", strerror(errno)));
            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("BaseSocket::close: close: ???:  ", socketId, " ", strerror(errno)));
        }
    }
    socketId = 0;
}

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

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

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

ConnectSocket::ConnectSocket(std::string const& host, int port)
    : DataSocket(::socket(PF_INET, SOCK_STREAM, 0))
{
    struct sockaddr_in serverAddr;
    bzero((char*)&serverAddr, sizeof(serverAddr));
    serverAddr.sin_family       = AF_INET;
    serverAddr.sin_port         = htons(port);
    serverAddr.sin_addr.s_addr  = inet_addr(host.c_str());

    if (::connect(getSocketId(), (struct sockaddr*)&serverAddr, sizeof(serverAddr)) != 0)
    {
        close();
        throw std::runtime_error(buildErrorMessage("ConnectSocket::ConnectSocket: connect: ", strerror(errno)));
    }
}

ServerSocket::ServerSocket(int port)
    : BaseSocket(::socket(PF_INET, SOCK_STREAM, 0))
{
    struct sockaddr_in serverAddr;
    bzero((char*)&serverAddr, sizeof(serverAddr));
    serverAddr.sin_family       = AF_INET;
    serverAddr.sin_port         = htons(port);
    serverAddr.sin_addr.s_addr  = INADDR_ANY;

    if (::bind(getSocketId(), (struct sockaddr *) &serverAddr, sizeof(serverAddr)) != 0)
    {
        close();
        throw std::runtime_error(buildErrorMessage("ServerSocket::ServerSocket: bind: ", strerror(errno)));
    }

    if (::listen(getSocketId(), 255) != 0)
    {
        close();
        throw std::runtime_error(buildErrorMessage("ServerSocket::ServerSocket: listen: ", strerror(errno)));
    }
}

DataSocket ServerSocket::accept()
{
    if (getSocketId() == 0)
    {
        throw std::logic_error(buildErrorMessage("ServerSocket::accept: accept called on a bad socket object (this object was moved)"));
    }

    struct  sockaddr_storage    serverStorage;
    socklen_t                   addr_size   = sizeof serverStorage;
    int newSocket = ::accept(getSocketId(), (struct sockaddr*)&serverStorage, &addr_size);
    if (newSocket == -1)
    {
        throw std::runtime_error(buildErrorMessage("ServerSocket:accept: accept: ", strerror(errno)));
    }
    return DataSocket(newSocket);
}

void DataSocket::putMessageData(char const* buffer, std::size_t size)
{
    std::size_t     dataWritten = 0;

    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("DataSocket::putMessageData: write: critical error: ", strerror(errno)));
                }
                case EDQUOT:
                case EFBIG:
                case EIO:
                case ENETDOWN:
                case ENETUNREACH:
                case ENOSPC:
                {
                    // Resource acquisition failure or device error
                    throw std::runtime_error(buildErrorMessage("DataSocket::putMessageData: write: resource failure: ", strerror(errno)));
                }
                case EINTR:
                        // TODO: Check for user interrupt flags.
                        //       Beyond the scope of this project
                        //       so continue normal operations.
                case EAGAIN:
                {
                    // Temporary error.
                    // Simply retry the read.
                    continue;
                }
                default:
                {
                    throw std::runtime_error(buildErrorMessage("DataSocket::putMessageData: write: returned -1: ", strerror(errno)));
                }
            }
        }
        dataWritten += put;
    }
    return;
}

void DataSocket::putMessageClose()
{
    if (::shutdown(getSocketId(), SHUT_WR) != 0)
    {
        throw std::domain_error(buildErrorMessage("HTTPProtocol::putMessageClose: shutdown: critical error: ", strerror(errno)));
    }
}

Answer 1

A public non-virtual destructor in a base class is risky. You should consider making it protected, or otherwise virtual, if there's need for polymorphic deletion of the BaseSocket.

---

I don't think 0 is a good value to indicate an uninitialized socket id. The socket function will return:

Upon successful completion, socket() shall return a non-negative integer, the socket file descriptor. Otherwise, a value of -1 shall be returned and errno set to indicate the error.

Zero is non-negative, so it could actually return the value of 0 for a valid socket. I would recommend defining a constant, InvalidSocketId and setting it to -1. Then you can use that constant inside your class to denote the invalid/uninitialized socket.

---

Don't repeat yourself, not even in the error messages.

if (getSocketId() == 0)
{
    throw std::logic_error(buildErrorMessage("DataSocket::getMessage: accept called on a bad socket object (this object was moved)"));
}

All functions that throw give a nice error message with the function name in it, and I commend you for taking that care, but writing down the name of the function as a string is a task that should be automated. By the way, notice that in the excerpt above the name in the error message (getMessage) isn't matching the name of the function throwing the exception (getMessageData). That's a very common little mistake we find in this pattern. The programmer will never remember to update the error messages when the name of the function changes.

YMMV, but such case seems like a valid case for a macro. Unfortunately, __func__ is not standard, but supported by GCC and Clang. MSVC provides the equivalent macro __FUNCTION__, which it appears is also supported by GCC for compatibility.

#define THORSANVIL_SOCKET_ERROR(exception, message) \
    throw exception{ buildErrorMessage(__FILE__, ":", __LINE__, " at ", __func__, ": ", message) }

THORSANVIL_SOCKET_ERROR(std::logic_error, "accept called on a bad socket object (this object was moved)");

Should print something like:

Socket.tpp:20 at DataSocket::getMessageData: accept called on a bad socket object (this object was moved)

---

Nitpickings

Please don't use bzero. It is very archaic and non-standard. You can use std::memset to the same effect, or better, call the default zero-initializing constructor: sockaddr_in serverAddr{}. While you are at it, you could also remove those struct tags in the variable declarations, so the code doesn't look like it was copied from some old C book ;).

Not so sure if domain_error has a place here. It is usually associated with numerical and mathematical errors. You could very well just go with runtime_errors there, or extend runtime_error with your own exception class(es).

Answer 2

I've been looking over your code because I wanted to do what you have done and write a nice thin C++ layer over the C socket APIs, so it's abstracted for modern C++ developers without having to worry about all the (more complex) gubbins that comes with using boost.ASIO

The only point I'd raise is that some names are too close to the C APIs for my liking, for example:

Sock::DataSocket accept = server.accept();

Could be more descriptive for a higher layer user? Something such as:

Sock::DataSocket client_connection = server.wait_client();

I dunno, it's a super minor point (especially as the call is blocking - it must wait for something to happen).... Great job BTW!