Is explicitly calling destructors from constructors bad practice in C++?

Question

I usually don't call destructors explicitly. But I'm designing TCP server class, which looks something like this:

class Server {
public:
    Server() {
        try {
            WSADATA wsaData;
            if (WSAStartup(MAKEWORD(2, 2), &wsaData))
                throw std::runtime_error("WSAStartup function failed.");
            ...

            if ((m_scListener = socket(pAddr->ai_family, pAddr->ai_socktype, pAddr->ai_protocol)) == INVALID_SOCKET)
                throw std::runtime_error("'socket' function failed.");
            ...
        }
        catch (std::exception& ex) {
            this->~Server();
            throw;
        }
    }

    ~Server() {
        if (m_scListener != INVALID_SOCKET) {
            closesocket(m_scListener);
            m_scListener = INVALID_SOCKET;
        }
        WSACleanup();
    }
private:
    SOCKET m_scListener = INVALID_SOCKET;
}

Is the code above considered as bad practice or design? What's recommended way of designing this? I wrote this way, because constructors can't return NULL. Should I make constructor private, and write static method that creates an instance of the Server class?

===== U P D A T E =====

OK, summarizing the answers, I came to this conclusion:

• Explicitly calling a destructor is generally a bad idea, even if it works as intended, this is something unusual, and other C++ programmers who will be dealing with your code may get confused with this approach. So it's best to avoid explicitly calling destructors.

• Breaking my original RAII class into micro RAII classes looks like a good solution. But I'm afraid that there are too many API calls in my real code that need for clean-ups (closesocket, CloseHandle, DeleteCriticalSection, etc...). Some of them are called only once and are never reused, and moving all of them into separate RAII classes seems too fanatical to me. This will also increase my code.

• In my opinion the most helpful answer is from M.M:

A better solution would be to keep the initialization code in the constructor, and call the cleanup function before throwing out.

Following the M.M's advice I rewrote my code this way:

class Server {
public:
    Server() {
        WSADATA wsaData;
        if (WSAStartup(MAKEWORD(2, 2), &wsaData))
            ThrowError("WSAStartup function failed.", true);
        ...

        if ((m_scListener = socket(pAddr->ai_family, pAddr->ai_socktype, pAddr->ai_protocol)) == INVALID_SOCKET)
            ThrowError("'socket' function failed.", true);
        ...
    }

    ~Server() { CleanUp(); }

private:
    SOCKET m_scListener = INVALID_SOCKET;

    void ThrowError(const char* error, bool cleanUp) {
        if (cleanUp)
            CleanUp();
        throw std::runtime_error(error);
    }

    void CleanUp() {
        if (m_scListener != INVALID_SOCKET) {
            closesocket(m_scListener);
            m_scListener = INVALID_SOCKET;
        }
        WSACleanup();
    }
};

I believe this design follows the RAII pattern, but only one class instead of 3-4 micro RAII classes.

Answer 1

Is explicitly calling destructors from constructors bad practice in C++?

Yes. If you call a destructor of an object that hasn't been constructed, the behaviour of the program is undefined.

Having undefined behaviour is a bad thing. It should be avoided whenever possible.

---

What's recommended way of designing this?

Follow the Single Responsibility Principle (SRP), and the Resource Aquisition Is Initialisation (RAII) pattern.

In particular, your Server has too many responsibilities. You should create a separate class that manages a socket. Within the constructor of that class, call scoket and within the destructor, call of that class, call closesocket. Maintain the class invariant that the contained socket always valid (closable) or INVALID_SOCKET and always unique if valid and is never leaked (i.e. the value is never overwritten without closing first). This is the RAII pattern.

Create a similar wrapper for wsa data.

Within Server, store members of these wrapper types. Server won't then need a custom destructor or other special member functions since those are handled by the members which manage themselves.

Answer 2

Destructors should only be called by a fully constructed object.

You can make an Init() and CleanUp() function instead of putting the setup code in the constructor. This will also make your Server object faster to construct.

class Server {
public:
    Server() = default;

    bool Init() {
      try {
            WSADATA wsaData;
            if (WSAStartup(MAKEWORD(2, 2), &wsaData))
                throw std::runtime_error("WSAStartup function failed.");
            ...

            if ((m_scListener = socket(pAddr->ai_family, pAddr->ai_socktype, pAddr->ai_protocol)) == INVALID_SOCKET)
                throw std::runtime_error("'socket' function failed.");
            ...
            return true;
        }
        catch (std::exception& ex) {
            return false;
        }
    }

    void CleanUp() {
        if (m_scListener != INVALID_SOCKET) {
            closesocket(m_scListener);
            m_scListener = INVALID_SOCKET;
        }
        WSACleanup();
    }

    ~Server() {
      CleanUp();
    }

private:
    SOCKET m_scListener = INVALID_SOCKET;
};

Caller-side code:

Server server;
if (!server.init()) {
   server.CleanUp();
}

Answer 3

What's recommended way of designing this?

I would say: even more RAII. Something like:

class WSARaii
{
public:
    WSARaii()
    {
        if (WSAStartup(MAKEWORD(2, 2), &wsaData))
            throw std::runtime_error("WSAStartup function failed.");
    }
    ~WSARaii()
    {
        WSACleanup();
    }
    WSARaii(const WSARaii&) = delete;
    WSARaii& operator =(const WSARaii&) = delete;

private:
    WSADATA wsaData;
};

class Socket
{
public:
    Socket(..) : m_scListener(socket(pAddr->ai_family, pAddr->ai_socktype, pAddr->ai_protocol) {
        if (m_scListener == INVALID_SOCKET)
            throw std::runtime_error("'socket' function failed.");
    }
    ~Server() {
        if (m_scListener != INVALID_SOCKET) {
            closesocket(m_scListener);
        }
    }
private:
    SOCKET m_scListener
};

And finally

class Server {
public:
    Server() : wsa(), socket(..) {}

private:
    WSARaii wsa;
    Socket socket;
};

Answer 4

I don't know what would happen there on a technical level, but it doesn't look good. I would recommend not doing that. It's far easier and less error-prone IMO to initialize high level systems like networking and whatnot inside a separate Init() method in your class. That way you can safely create an instance, call its Init() method, check the result, and delete (or call a Destroy(), or both) on failure.

I would only assign default values inside the constructor and let outside code call your destructors with delete.

Answer 5

Is the code above considered as bad practice or design?

Yes calling consrtuctor or destructor explicitly is almost always wrong, except very seldom cases and this is not the one.

What's recommended way of designing this?

Recommended way is to use RAII. In this case you can use std::unique_ptr with custom deleter which calls closesocket() etc. Or you can create your own wrapper. Then you can safely throw exception and make sure that resources that initialized get cleaned properly.

Answer 6

Taking a look at your design, you have this code in the socket() call in the constructor:

pAddr->ai_family, pAddr->ai_socktype, pAddr->ai_protocol.

What if the user of the Server class wants to use different socket type, protocol, etc. before the socket() is opened? They have no recourse, as they are locked into the values you are using in pAddr (you never mentioned where you are getting these values, but they certainly are being set prior or within the Server constructor).

If you made those socket arguments individual members of the class, that opens up the class design so that it is not necessary to invoke an ill-conceived call to the destructor, since the constructor would not get involved in calling socket() or even WSAStartup.

class Server 
{
    public:
        void set_family(int family) { m_family = family; }
        //.. other setters

        void start()
        {
            WSADATA wsaData;
            if (WSAStartup(MAKEWORD(2, 2), &wsaData))
                throw std::runtime_error("WSAStartup function failed.");

            if ((m_scListener = socket(m_family, m_type, m_protocol)) == INVALID_SOCKET)
                throw std::runtime_error("'socket' function failed.");
        }

        void stop()  
        {
            if (m_scListener != INVALID_SOCKET) 
            {
                closesocket(m_scListener);
                m_scListener = INVALID_SOCKET;
            }
            WSACleanup();
        }

        ~Server() noexcept
        {
           try 
           { 
              stop();
           }
           catch(...) { } // add any additional catches above this, 
                          // but make sure no exceptions escape the destructor
       }  

    private:
        SOCKET m_scListener = INVALID_SOCKET;
        int m_family = AF_INET;
        int m_type = SOCK_STREAM;
        int m_protocol = IPPROTO_TCP;
 };

This (to me) is a cleaner and more flexible interface that does not need to have destructors called explicitly. The actual initialization of WinSock and connection to the socket is only done in the start() call.

In addition, the parameters to socket are member variables that are initialized to basic values, but can be changed with the set... functions before the call to Server::start().

The other addition is the try/catch within the destructor of Server. Note that this was done to ensure that anything that can be thrown does not escape the destructor call, else a std::terminate would be invoked.