Correctly applying the "rule of five" to a RAII socket wrapper

Question

I was trying to create a simple RAII wrapper with rule of 5 for a TCP POSIX socket. My aim was to try learn how to apply rule of five in different situations, but this one was somehow tricky.

class tcp_socket {
public:
    tcp_socket() {
        _s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    }

    tcp_socket(const tcp_socket& other) = delete;
    tcp_socket& operator=(const tcp_socket& other) = delete;

    tcp_socket(tcp_socket&& other) noexcept {
        _s = other._s;
        other._s = -1;
    }

    tcp_socket& operator=(tcp_socket&& other) noexcept {
        if (this != &other) {        
            _s = other._s;
            other._s = -1;
        }
        return (*this);
    }

    ~tcp_socket() {
        close(_s);
    }

private:
    int _s = -1;
};

In this class, I like to create socket in constructor and close it in destructor, a logical RAII approach. But I noticed closing socket in destructor creates a lot of side effects for writing other 4 routines of rule of five.

Move constructor/assignment:

As you see, I use other._s = -1; in move constructor/assignment. Because if I don't, when the moved object deletes, it will also close my socket in moved-to object. I think this will solve my problem, because when moved object will be destroyed, it will try to close -1 file descriptor which is an error, but will not ruin anything.

Copy constructor/assignment:

As you see, the only method that I could think of for copy constructor/assignment is deleting them. Because at first I thought to use dup() to duplicate file descriptor for copying, but if I do so, operations on one one instance (for example shutdown()) will affect another instance.

What do you think about this class? Do you think if this class does provide a simple RAII with correct rule of five or I need to adjust it somehow? Is it possible to write Copy constructor/assignment somehow or it is not logical to provide them?

Answer 1

These are good questions!

1. You should indeed have the copy constructor & assignment operator deleted.

The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-assigned - only moved. An example of this: std::unique_ptr.

It's also what I would recommend in your case, because:

1. Like you said, duplicating a file descriptor is weird and unexpected.
2. It's not obvious to the user of your class what the copy behavior should be.
3. There don't seem to be - AFAICT - common scenarios in which you would copy, rather than move or pass by reference, a TCP socket.

2. Consider using std::optional to indicate "no valid value" or "missing" or "none"

In C (and the C system call bindings on Unix-like systems), it is a convention to use the -1 value for an invalid/missing file descriptor is a convention. We know that the int type is actually larger than the actual space of possible file descriptor values, so we use a junk value, which we assume the OS never uses, to indicate "no valid value". Now, this works fine; and you could choose to, say, define a static class constant:

static constexpr const int no_file_descriptor { -1 };

and then write:

close(_s);
_s = other._s;
other._s = no_file_descriptor;

_{As @HolyBlackCat suggests, you must get _s to be closed somehow. You could also just swap the two descriptors, but I find that to contradict the element of least surprise.}

but you might want to consider the more general (though less space-efficient) solution, which is the std::optional<T> type template. It is intended for exactly your case: Either holding some value of type T (in your case, int), or holding some indication of "no value". Using an optional, you could write:

close(_s.get());
_s = other._s;
other._s = std::nullopt;

You'll still need to write your move assignment and move construction code, unfortunately (thanks @CassioRenan for noticing these are both necessary).

Other suggestions

3. Don't use a plain int; either find a type definition of a file descriptor from some library you're using, or if you have no definition to borrow, have using file_descriptor = int; or using file_descriptor_index = int.
4. _s is a bad member name. Use something more explicit, e.g. descriptor_index_ or posix_descriptor_index_.
5. Always check the return value of library/system calls! And handle errors.

The modified code

class tcp_socket {
protected:
    void close_if_neccessary() {
       constexpr auto socket_close_failed { -1 };
        if (descriptor_index_.has_value()) { 
            auto retval = close(descriptor_index_.value()); 
            if (retval == socket_close_failed) { 
               // throw something here, e.g.:
               // throw std::system_error(errno, std::system_category(), "close()");
            }
    };
public:
    using file_descriptor_index = int;

    tcp_socket() : descriptor_index_() { 
       constexpr auto socket_creation_failed { -1 };
       auto retval = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
       if (retval == socket_creation_failed) { 
           // throw std::system_error(errno, std::system_category(), "socket()");
       }
       descriptor_index = retval;
    };

    tcp_socket(const tcp_socket& other) = delete;
    tcp_socket& operator=(const tcp_socket& other) = delete;
    tcp_socket& operator=(tcp_socket&& other) noexcept {
        if (other.descriptor_index_ != descriptor_index_) {
            close_if_neccessary();
            descriptor_index_ = other.descriptor_index_;
            other.descriptor_index_ = std::nullopt;
        }
        return this;
    };
    tcp_socket(tcp_socket&& other) noexcept :
        descriptor_index_(other.descriptor_index_) 
    {
        other.descriptor_index = std::nullopt;
    }
    ~tcp_socket() { 
        // You might want to wrap this in a try-catch, since
        // destructors shouldn't throw. Otherwise you're risking
        // a double-exception and immediate program termination.
        close_if_necessary(); 
    }

protected:
    std::optional<file_descriptor_index> descriptor_index_;
};

See this StackOverflow question about the weird exception code in the comments:

Answer 2

There's one major problem with the code:

Your move assignment is broken.

You forgot to close(_s) before overwriting it with other._s.

To avoid this kind of problems, I suggest using the copy-and-swap idiom. It makes writing a operator= a no-brainer in most cases:

tcp_socket &operator=(tcp_socket other) noexcept // Note the lack of `&&`.
{
    std::swap(_s, other._s);
    return *this;
}

If you decide to do this, you also need to remove tcp_socket& operator=(const tcp_socket& other) = delete; to prevent it from conflicting with this operator.

---

Additionally...

You shouldn't do close(-1).

Moving an object sets _s of the original object to -1.

close(-1) is not a no-op. (It sets errno to 'bad file descriptor'.) Thus the destructor should do

if (_s != -1)
    close(_s);

It would be a good idea to have a way to create a 'null' tcp_socket instance.

Instances of tcp_socket don't necessarily own sockets. By moving from an instance, or make it 'null' (i.e. it no longer owns a socket).

IMO, it would make sense to have a way to directly create 'null' intstances.

I wouldn't open the socket in socket(), and create a separate constructor that does open one.

You don't have to explicitly delete the copy constructor and assignment operator.

Declaring a move constructor or assignment operator causes both the constructor and assignment operator to be implicitly deleted.

Whether or not you should delete them explicitly (for extra clarity) is a different question.

Answer 3

As other answers have said, use swap to get assignment correct, and don't close already-closed descriptors.

Some minor style nitpicks to add:

• Use initializers in preference to assignment in the constructors.
• There's no need to name the arguments to the deleted copy methods.
• Remove the redundant parens from return *this;.
• Consider a public close() method, for users that care about handling errors.

In contrast to einpoklum, I like your choice of invalid file descriptor (-1); we don't need the overhead of std::optional when a clear invalid value is available. I do recommend giving it a name, though.

My version

#include <utility>

#include <unistd.h>
#include <sys/types.h>          /* required on pre-POSIX BSD platforms */
#include <sys/socket.h>
#include <netinet/in.h>

class tcp_socket
{
public:
    tcp_socket()
        : s{::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)}
    {
    }

    tcp_socket(const tcp_socket&) = delete;
    void operator=(const tcp_socket&) = delete;

    tcp_socket(tcp_socket&& other) noexcept
        : s{other.s}
    {
        other.s = null_socket;
    }

    tcp_socket& operator=(tcp_socket&& other) noexcept
    {
        return swap(other);
    }

    int close() {
        return ::close(s);
    }
    
    ~tcp_socket()
    {
        if (s != null_socket) {
            // note: errors are ignored!
            close();
        }
    }

private:
    static constexpr int null_socket = -1;

    int s;

    tcp_socket& swap(tcp_socket& other) noexcept
    {
        std::swap(s, other.s);
        return *this;
    }
};

Future directions

You might need a udp_socket before long. It's probably a good idea to create a socket base class with protected constructor so that the RAII is managed in a single-responsibility class and the protocol-specific part is in relevant subclasses:

class socket
{
protected:
    socket(int fd)
        : s{fd}
    {
    }

public:
    // ...
};

class tcp_socket : public socket
{
public:
    tcp_socket()
        : socket{::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)}
    {
    }
};