12
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – PaulMcKenzie Dec 3 '19 at 19:30
  • \$\begingroup\$ @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. \$\endgroup\$ – Afshin Dec 3 '19 at 19:33
  • \$\begingroup\$ @SombreroChicken the main reason I posted it here was copy constructor/assignment and if there is a way to provide them. \$\endgroup\$ – Afshin Dec 3 '19 at 19:33
  • \$\begingroup\$ @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 \$\endgroup\$ – Guillaume Racicot Dec 3 '19 at 19:34
  • \$\begingroup\$ @Afshin -- Change the word "socket" to "stream" in your comment, and you will see yourself in the same boat as C++ streams. \$\endgroup\$ – PaulMcKenzie Dec 3 '19 at 19:35
7
\$\begingroup\$

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:

\$\endgroup\$
  • \$\begingroup\$ if I use std::optional, I need to check and if it has value then close it is destructor,isn't it? \$\endgroup\$ – Afshin Dec 3 '19 at 19:54
  • 2
    \$\begingroup\$ @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. \$\endgroup\$ – Guillaume Racicot Dec 3 '19 at 20:26
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Cássio Renan Dec 3 '19 at 21:34
  • 1
    \$\begingroup\$ @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??? \$\endgroup\$ – E. Vakili Dec 4 '19 at 6:51
  • 3
    \$\begingroup\$ @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. \$\endgroup\$ – E. Vakili Dec 4 '19 at 9:51
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – einpoklum Dec 3 '19 at 19:59
  • \$\begingroup\$ 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. \$\endgroup\$ – Afshin Dec 3 '19 at 20:02
  • \$\begingroup\$ @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.) \$\endgroup\$ – HolyBlackCat Dec 3 '19 at 20:04
  • \$\begingroup\$ @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. \$\endgroup\$ – HolyBlackCat Dec 3 '19 at 20:11
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – HolyBlackCat Dec 3 '19 at 21:12
4
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.