# Correctly applying the “rule of five” to a RAII socket wrapper

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?

• I don't see anything wrong with deleting the copy constructor and assignment operator if they don't make sense. The C++ stream classes do the same thing. – PaulMcKenzie Dec 3 '19 at 19:30
• @PaulMcKenzie The only situation for copy constructor/assignment that I think of is when you was to use single socket in for example multiple thread. You want to pass socket for example in a visitor approach to other classes and use it in other classes. SO copy constructor/assignment is useful, but I really could not find a proper way to provide them. – Afshin Dec 3 '19 at 19:33
• @SombreroChicken the main reason I posted it here was copy constructor/assignment and if there is a way to provide them. – Afshin Dec 3 '19 at 19:33
• @Afshin in that case you need something like tcp_socket_ref which act as an observer to the socket. Much like you can use raw pointers to observe the value of a std::unique_ptr – Guillaume Racicot Dec 3 '19 at 19:34
• @Afshin -- Change the word "socket" to "stream" in your comment, and you will see yourself in the same boat as C++ streams. – PaulMcKenzie Dec 3 '19 at 19:35

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

1. 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.
2. _s is a bad member name. Use something more explicit, e.g. descriptor_index_ or posix_descriptor_index_.
3. 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:

• if I use std::optional, I need to check and if it has value then close it is destructor,isn't it? – Afshin Dec 3 '19 at 19:54
• @einpoklum-reinstateMonica because there is a complete subset of a value an int can take to represent a unix socket. -1 is the value used to contain the no socket in case of errors. I would personally not transform the value representation of what an API returns. I may not know someday -2 could be used for something else. Also, it's a bit like std::unique_ptr. It doesn't contain an optional pointer. Rather, the nullptr value denotes the not owner value. – Guillaume Racicot Dec 3 '19 at 20:26
• Just noticed something else: If you use the move constructor by doing tcp_socket a; tcp_socket b(std::move(a)), you end up closing the same socket twice. This happens because when moving a std::optional, the moved-from optional will still contain a value (except that it will be a moved-from value). My take from this is that std::optional is really not a great choice for this specific problem. – Cássio Renan Dec 3 '19 at 21:34
• @einpoklum-reinstateMonica "It's much better to have simpler, shorter, code with less information to remember.", and that is what you did in this code??? – E. Vakili Dec 4 '19 at 6:51
• @einpoklum-reinstateMonica Adding std::optional does not add any benefits to the solution except loss of efficiency, readability, and ... It's not a C++ point of view as you claim. C++ aims at bringing strong abstraction alongside the efficiency. Don't pay for what you don't need. – E. Vakili Dec 4 '19 at 9:51

There's one major problem with the code:

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.

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.

• I would +1 you for the first two suggestions, but -1 you for the third one. It would not be a good idea to be able to create a 'null' TCP socket. That kind of flies in the face of the RAII approach. Now, you could argue that perhaps RAII is not a good approach for sockets, but that's a different discussion. – einpoklum Dec 3 '19 at 19:59
• if you provide move assignment in the way you did, you practically provide copy assignment too.isn't it? so I think it is better to add && to provide move only. and yea, I missed close(_s);, I should have not made that mistake. – Afshin Dec 3 '19 at 20:02
• @Afshin If you had a non-deleted copy constructor, then yes, this assignment operator would act as a copy assignment too (another reason why copy-and-swap is so good). But since your copy constructor is deleted, it doesn't. If you added && here, you'd need to do other._s = -1 manually. (Having operator= swap objects is not a good idea.) – HolyBlackCat Dec 3 '19 at 20:04
• @einpoklum-reinstateMonica Let's agree to disagree. :) From my experience, it's extremely convenient (and it would be even more convenient if creating a socket required some parameters, preventing tcp_socket from being default-constructible otherwise). And, after all, that's what std::fstream does. – HolyBlackCat Dec 3 '19 at 20:11
• @Afshin More function calls doesn't necessarily mean slower code, the only way to know for sure is to benchmark it. I wouldn't expect any noticeable slowdown, especially in release builds. The reason why I prefer the &&-less version is because it's easier to write. – HolyBlackCat Dec 3 '19 at 21:12

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)}
{
}
};