C++ Copy constructor of object owning a POSIX file descriptor

Question

I need to put C++ objects owning POSIX File descriptors in vector. I have a vector of filepaths which I use to create my objects. Here is my code:

main.cpp

std::vector<MightyObject> mightyObjects;
std::vector<const char*> paths = {"awesomePath1", "awesomePath2"};
for (std::vector<const char*>::iterator it = paths.begin(); it != paths.end(); ++it)
{
    mightyObjects.emplace_back(MightyObject(*it));
}

MightyObject.cpp

MightyObject::MightyObject(const char* path)
{
   this->fd = open(path, O_RDWR | O_NONBLOCK);
}

MightyObject::~MightyObject()
{
    close(this.fd);
}

MightyObject::MightyObject(const MightyObject& obj)
{
    this->fd = dup(obj.fd);
}

MightyObject& MightyObject::operator=(const MightyObject& other)
{
    this->fd = dup(other.fd);
    return *this;
}

In mightyObjects, I have objects with wrong file descriptors...

What am I doing wrong ?

don't forget the rule of 3: you have to define a proper assignment operator too. — Jean-François Fabre, Dec 12 '16 at 15:40
I think `emplace_back` has no effect like this. Should be `mightyObjects.emplace_back(*it);`. And your assignment operator "leaks" because you don't close the previous descriptor. Using the swap idiom is recommended here (with `std::move`) — Jean-François Fabre, Dec 12 '16 at 15:42
How would my mightyObjects vector be populated with MightyObjects instances in such a case ? Tried push_back instead. Same result. — MichelPeseur, Dec 12 '16 at 15:44
@imreal Because I have two differents file and my 2 MightyObject have the same fd. — MichelPeseur, Dec 12 '16 at 15:45
You need to close the old `this->fd` in the assignment operator. Also, this is an excellent use case for the move constructor, where the move operation would "steal" the file descriptor of the right-hand side without any dup-ing or close-ing. — user4815162342, Dec 12 '16 at 15:47
@Jean-FrançoisFabre so I should close the descriptor after the dup ? — MichelPeseur, Dec 12 '16 at 15:47
@user4815162342 I suppose I have a to learn how to use move constructor, so ? — MichelPeseur, Dec 12 '16 at 15:48
Also, your assignment operator doesn't handle self-assignment (the case when `this == &other`), in which case it should do nothing. — user4815162342, Dec 12 '16 at 15:48
Can you show some short example code that proves this doesn't work? — Galik, Dec 12 '16 at 15:50
@Galik Copy semantics works perfectly fine as long as you are able to `dup` the descriptor. It's like a reference-counting scheme provided by the kernel. — user4815162342, Dec 12 '16 at 15:58
@user4815162342 Ah, my man page doesn't mention that. It just says they may be used *interchangeably* so I assumed closing one would close both. — Galik, Dec 12 '16 at 16:05
@MichelSardou `so I should close the descriptor after the dup ?` No, you should close `this->fd` **before** you reassign it with the descriptor returned by `dup`. After that the old descriptor is gone (and leaked unless you closed it). — eerorika, Dec 12 '16 at 16:06
@Galik It would still be a good idea to implement move semantics to avoid unnecessary dup-ing of file descriptors. I've added an answer to that effect. — user4815162342, Dec 12 '16 at 16:06
"Owning a file handle" is a vacuous concept. One can own a file behind a handle, but a handle itself is about as meaningful as a pointer to an object. Objects may own other objects, but no one owns pointers. It makes sense to keep file descriptors in uncopyable, unmovable objects, not too dissimilar with stdio FILEs, and pass around (smart) pointers to such objects. You get all the benefits of the rule of zero this way. — n. 'pronouns' m., Dec 12 '16 at 16:59
@n.m. "Owning" in the sense of "being responsible to dispose of" is perfectly defined in C++. Either way, I agree that allocating a new file descriptor for every copy ctor invocation is a bad idea. — user4815162342, Dec 12 '16 at 17:40
@user4815162342 In addition to being defined in C++ things need to make sense. The latter is harder to come by than the former. — n. 'pronouns' m., Dec 12 '16 at 17:53

score 7 · Accepted Answer · edited May 23 '17 at 12:00

The code as shown should contain correct file descriptors, although it will leak descriptors on assignments. To fix it, you should change it to:

Fix the leak in the assignment operator;
Handle self-assignment;
Implement a move constructor to optimize moves (and because this class is a textbook example of benefits of move construction);
Handle errors.

For example (untested):

static int safe_dup(int fd) {
    int copy = dup(fd); 
    if (copy < 0)
        throw std::runtime_error(strerror(errno));
    return copy;
}

MightyObject::MightyObject(const char* path) {
    this->fd = open(path, O_RDWR | O_NONBLOCK);
    if (this->fd == -1)
        throw std::runtime_error(strerror(errno));
}

MightyObject::~MightyObject() {
    if (this->fd != -1)
        close(this->fd);
}

MightyObject::MightyObject(const MightyObject& other) {
    this->fd = safe_dup(other.fd);
}

MightyObject& MightyObject::operator=(const MightyObject& other) {
    if (this != &other) {
        close(this->fd);
        this->fd = safe_dup(other.fd);
    }
    return *this;
}

MightyObject::MightyObject(MightyObject&& other) {
    // "move" file descriptor from OTHER to this: no duping is needed,
    // we just take the descriptor and set the one in OTHER to -1, so
    // it doesn't get closed by OTHER's destructor.
    this->fd = other.fd;
    other.fd = -1;
}

MightyObject& MightyObject::operator=(MightyObject&& other) {
    // move assignment operator
    close(this->fd);
    this->fd = other.fd;
    other.fd = -1;
}

Once you understand the move semantics, you can reduce the code repetition between the operators by deploying the copy and swap idiom.

Note that the above should be understood as a coding and understanding exercise. In production you likely wouldn't want to allocate a new file descriptor whenever your object is being copied around. A better design would make the file handle move-only and to implement MightyObject as std::shared_ptr<Handle>. That will completely avoid dup and fiddling around with the copy constructor, assignment operator, etc., because all of them will be handled by shared_ptr.

In addition to adding a move constructor, don't forget to add a move assignment operator as well. Or, you can change the existing assignment operator to handle both copy and move semantics with a single code. — Remy Lebeau, Dec 12 '16 at 19:34
@RemyLebeau I've now added a move assignment operator as well. The copy/swap idiom has less repetition and is therefore the preferred option for production code, but is unfortunately not as clear for introducing the concept to a beginner. — user4815162342, Dec 13 '16 at 09:38
the move assignment operator should be returning `*this`, just like the copy assignment operator. — Remy Lebeau, Dec 13 '16 at 15:26

C++ Copy constructor of object owning a POSIX file descriptor

1 Answers1