0

In code which wrap a resource which should be freed once and only once, is it idiomatic to do something like the following to guarantee this? Is there a superior approach?

class SocketWrapper { 
    SocketWrapper() {
        fd = socket(AF_INET, SOCK_STREAM, 0);
    }

    ~SocketWrapper() {
        if(fd < 0){
            close(fd);
        }
    }

    SocketWrapper(SocketWrapper &&other){
        fd = other.fd;
        other.fd = -1;
    }
    //similar move assignment   

private: 
    int fd{-1};
};
cloakedlearning
  • 197
  • 6
  • 2
    You should probably use -1 because 0 is a valid FD. – ysdx Jul 04 '16 at 23:01
  • Thanks, I'll edit that in. – cloakedlearning Jul 04 '16 at 23:05
  • Yes, it is idiomatic. No, I don't know of a superior approach. Note, I agree with @ysdx except I would say use whatever invalid value is specified for the target platform (`INVALID_SOCKET`, `-1`, etc). – James Adkison Jul 04 '16 at 23:09
  • Then you should change the check in the dtor because any nonzero int evaluates to true. That said, in cases like this where the members have low complexity, I'd define the move ctor in terms of the move assignment operator - you guarantee they match in that case. – Javier Martín Jul 04 '16 at 23:10
  • I think you'll want to fix your destructor now that you've updated the initial value for `fd`. – James Adkison Jul 04 '16 at 23:10
  • I also think that's fine for simple uses. For more complicated situations (for example where several sub-components might share a socket and neither be the singular owner of the resource) I've often had a 'Socket' class that has a shared pointer to a 'SocketFdResource' object which owns and is responsible for closing the filedescriptor. Once socket objects are shared, I find this necessary to keep usage simple. – Wuggy Jul 04 '16 at 23:16
  • @wuggy, nice idea, I can see how that could help reduce complexity – cloakedlearning Jul 04 '16 at 23:23

2 Answers2

0

You should probably delete the copy constructor for the class as well to be safe. 

SocketWrapper(const SocketWrapper&) = delete;

This should be safe to do but I guess if you want the resource to be created once as well you may use a singleton. There are libraries which provide singleton functionalities out of the box. For ex: - Folly singleton.

Also, its always advisable to reuse libraries which provide exiting wrappers over primitive sockets and are extensively tested and hence robust to use instead of rewriting the whole wrapping folly's AsyncSocket which provides similar abstractions unless you want just basic fd wrapping functionality.

Prateek Jain
  • 96
  • 5
  • 3
    Once the move constructor is declared the copy constructor is implicitly deleted. It does no harm to also explicitly delete it, but it isn't necessary. – Howard Hinnant Jul 04 '16 at 23:16
  • I thought that defining the move ctor/assignment operator implicitly deleted the copy ctor/assignment? – cloakedlearning Jul 04 '16 at 23:16
0

I'd like to commend you use similar strategy as smart pointer. Both shared_ptr and unique_ptr are used to managed the pointer resource.

For different numbers of owner you could choose one. Ref:Differences between unique_ptr and shared_ptr

Community
  • 1
  • 1
Tongxuan Liu
  • 270
  • 1
  • 11