2

I have a class which manages a resource (a network socket).

I have written a class ConnectionHandler which handles a network socket created from a call to accept().

This class is designed with RAII in mind, when accept() is called, the returned socket is placed into a ConnectionHandler, when this goes out of scope the destructor closes the socket.

I'm also keeping track of all my open ConnectionHandler's by saving them in a map (Maps the socket address (IP:Port) to the ConnectionHandler that corresponds to that address).

I'm having a problem "emplacing" these ConnectionHandler's into the map though.

I've made it so that a ConnectionHandler can't be copied (at least I believe I've made it so), but when calling std::map::emplace, the ConnectionHandler's destructor is called (presumably to delete a temporary object created somewhere along the line) and the socket is closed.

As you can see, this creates a problem, because now the socket can't be used further down the program.

Is there any way for me to prevent the ConnectionHandler's destructor being called when emplacing it into a std::map?

Here's the code for the ConnectionHandler: Header file:

class ConnectionHandler
{
    private:
        constexpr static long BUFFER_SIZE = 1 << 12;    // 4K Buffer

        SocketAddress peer;             // This is kept around to be able to produce clear exception messages when something goes wrong
        SocketFileDescriptor socket;    // using SocketFileDescriptor = int;

    public:
        ConnectionHandler() noexcept = delete;                                      // Default Constructor

        explicit ConnectionHandler(SocketFileDescriptor socket, const SocketAddress& socketAddress) noexcept;   // Value Constructor

        ConnectionHandler (ConnectionHandler&& handler) noexcept;                   // Move Constructor

        ConnectionHandler (const ConnectionHandler& handler) = delete;              // Delete Copy Constructor

        ConnectionHandler& operator= (ConnectionHandler&& handler) noexcept;        // Move Assignment Operator

        ConnectionHandler& operator= (const ConnectionHandler& handler) = delete;   // Delete Copy Assignment Operator

        ~ConnectionHandler();                                                       // Destructor

        void close() noexcept;                                                      // Allow the owner to manually close the socket if necessary

        void set_blocking (bool blocking) const;                                    // Make the socket either blocking or non-blocking

        friend std::ostream& operator<< (std::ostream& stream, const ConnectionHandler& handler);   // Receive data from the socket

        friend std::istream& operator>> (std::istream& stream, const ConnectionHandler& handler);   // Send data to the socket
};

And the implementation:

ConnectionHandler::ConnectionHandler(SocketFileDescriptor socket, const SocketAddress& socketAddress) noexcept: peer(socketAddress), socket(socket)
{
}

ConnectionHandler::ConnectionHandler(ConnectionHandler&& handler) noexcept: peer(std::move(handler.peer)), socket(handler.socket)
{
}

ConnectionHandler& ConnectionHandler::operator=(ConnectionHandler&& handler) noexcept
{
    this->peer = std::move(handler.peer);
    this->socket = handler.socket;
    return *this;
}

ConnectionHandler::~ConnectionHandler()
{
    if (this->socket > 0)   //  Check if the socket has been closed manually
                            //  Don't bother setting the socket to -1, the object is being destroyed anyway
    {
        std::cout << "Closing socket from destructor " << this->socket << std::endl;
        ::close(this->socket);
    }
}

void ConnectionHandler::close() noexcept
{
    std::cout << "Closing socket from close() " << this->socket << std::endl;   // Close the socket manually and indicate it is closed by setting it's value to -1
    ::close(this->socket);
    this->socket = -1;
}

[...]

This is what the SocketAddress class looks like (Doesn't work for IPv6, I'm aware):

class SocketAddress
{
    private:
        std::array<std::uint8_t, 4> ip;
        std::uint16_t port;

    public:
        friend void swap (SocketAddress& sa1, SocketAddress& sa2) noexcept;

        SocketAddress() noexcept;

        explicit SocketAddress(struct sockaddr_storage* sockaddrStorage);

        SocketAddress (const SocketAddress& address) = default;

        SocketAddress (SocketAddress&& address) noexcept = default;

        SocketAddress& operator= (SocketAddress address);

        friend bool operator< (const SocketAddress& lhs, const SocketAddress& rhs) noexcept;

        friend std::string to_string(const SocketAddress& address) noexcept;
};

And finally, here's the code that creates the ConnectionHandler and places it in a map:

void Server::listenLoop()   // acceptLoop() would be a better name
{

    struct sockaddr_storage remoteAddr;

    while(!stop)    // stop is a std::atomic<bool>
    {
        [...]   // accept() connections in a loop

        SocketAddress address = SocketAddress(&remoteAddr);
        this->incomingSockets.emplace(std::make_pair(address, ConnectionHandler(childFileDesc, address)));
    }

    [...]
}

This function runs on a thread separate from the main thread, the thread is kept in the Server object and is joined in the Server object's destructor.

1
  • 1
    Please note that zero is a valid filedescriptor, so in ~ConnectionHandler(), use if (socket != -1).
    – G. Sliepen
    Commented Oct 21, 2018 at 13:01

2 Answers 2

5

In your move constructor/assignment operator you need to invalidate the moved from objects. The destructors will still be called on the moved from objects. If their socket is not 0 then the destructor will still call close on the fd.

1
  • insted of the original, put «socket{std::exchange(handler.socket,-1)}» in mv ctor and «this->socket=std::exchange(handler.socket,-1)» in mv assign.
    – Red.Wave
    Commented Oct 21, 2018 at 13:57
1

Your move operations are broken as they leave you with two objects referencing the same socket. You need to put a dummy (invalid) socket value to the moved-from object, and check for that in the destructor.

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.