What's the best practice to prevent memory leak if an exception thrown in constructor?

Question

I know if an exception is thrown in constructor, destructor will not be called(simple class, no inheritance). So if an exception is thrown in constructor and there is a chance some heap memory is not cleaned up. So what's best practice here? let's assume I have to call some function in constructor and it may throw exception. Shall I always use shared pointer in this case? What's the alternatives? Thank you!

Answer 1

I would stick to the RAII idiom.

If you avoid "naked" resources (like operator new, naked pointers, naked mutexes, etc.) and instead wrap everything into a container or class with proper RAII behavior you will not have the problems you describe, even in the presence of exceptions.

That is, do not acquire naked resources in your constructor. Instead, create an instance of an object which itself follows RAII. That way, even if your constructor fails (that is, the one which creates the instances), the destructors of the objects which were initialized will be called.

So, this is bad practice:

#include<iostream>
#include<stdexcept>

struct Bad {
  Bad() {
    double *x = new double;
    throw(std::runtime_error("the exception was thrown"));
  }

  ~Bad() {
    delete x;
    std::cout<<"My destructor was called"<<std::endl;
  }

  double *x;  
};

int main() {
  try {
    Bad bad;
  } catch (const std::exception &e) {
    std::cout<<"We have a leak! Let's keep going!"<<std::endl;
  }
  std::cout<<"Here I am... with a leak..."<<std::endl;
  return 0;
}

Output:

We have a leak! Let's keep going!
Here I am... with a leak...

Compare with this contrived and silly good implementation:

#include<iostream>
#include<stdexcept>

struct Resource {

  Resource() {
    std::cout<<"Resource acquired"<<std::endl;    
  }

  ~Resource() {
    std::cout<<"Resource cleaned up"<<std::endl;        
  }

};

struct Good {
  Good() {
    std::cout<<"Acquiring resource"<<std::endl;
    Resource r;
    throw(std::runtime_error("the exception was thrown"));
  }

  ~Good() {
    std::cout<<"My destructor was called"<<std::endl;
  }  
};


int main() {
  try {
    Good good;
  } catch (const std::exception &e) {
    std::cout<<"We DO NOT have a leak! Let's keep going!"<<std::endl;
  }
  std::cout<<"Here I am... without a leak..."<<std::endl;
  return 0;
}

Output:

Acquiring resource
Resource acquired
Resource cleaned up
We DO NOT have a leak! Let's keep going!
Here I am... without a leak...

My point is the following: try to encapsulate all resources which need to be liberated into their own class where the constructor does NOT throw, and the destructor correctly releases the resource. Then, on the other classes where the destructor may throw, simply create instances of the wrapped resource and the destructors of the acquired resource wrappers will be guaranteed to clean-up.

The following is probably a better example:

#include<mutex>
#include<iostream>
#include<stdexcept>

// a program-wide mutex
std::mutex TheMutex;

struct Bad {
  Bad() {
    std::cout<<"Attempting to get the mutex"<<std::endl;
    TheMutex.lock();
    std::cout<<"Got it! I'll give it to you in a second..."<<std::endl;
    throw(std::runtime_error("Ooops, I threw!"));
    // will never get here...
    TheMutex.unlock();
    std::cout<<"There you go! I released the mutex!"<<std::endl;    
  }  
};

struct ScopedLock {
  ScopedLock(std::mutex& mutex)
      :m_mutex(&mutex) {
    std::cout<<"Attempting to get the mutex"<<std::endl;
    m_mutex->lock();
    std::cout<<"Got it! I'll give it to you in a second..."<<std::endl;    
  }

  ~ScopedLock() {
    m_mutex->unlock();
    std::cout<<"There you go! I released the mutex!"<<std::endl;        
  }
  std::mutex* m_mutex;      
};

struct Good {
  Good() {
    ScopedLock autorelease(TheMutex);
    throw(std::runtime_error("Ooops, I threw!"));
    // will never get here
  }  
};


int main() {
  std::cout<<"Create a Good instance"<<std::endl;
  try {
    Good g;
  } catch (const std::exception& e) {
    std::cout<<e.what()<<std::endl;
  }

  std::cout<<"Now, let's create a Bad instance"<<std::endl;
  try {
    Bad b;
  } catch (const std::exception& e) {
    std::cout<<e.what()<<std::endl;
  }

  std::cout<<"Now, let's create a whatever instance"<<std::endl;
  try {
    Good g;
  } catch (const std::exception& e) {
    std::cout<<e.what()<<std::endl;
  }

  std::cout<<"I am here despite the deadlock..."<<std::endl;  
  return 0;
}

Output (compiled with gcc 4.8.1 using -std=c++11):

Create a Good instance
Attempting to get the mutex
Got it! I'll give it to you in a second...
There you go! I released the mutex!
Ooops, I threw!
Now, let's create a Bad instance
Attempting to get the mutex
Got it! I'll give it to you in a second...
Ooops, I threw!
Now, let's create a whatever instance
Attempting to get the mutex

Now, please don't follow my example and create your own scope guard. C++ (specially C++11) are designed with RAII in mind and provide a wealth of lifetime managers. For example, an std::fstream will automatically close, an [std::lock_guard][2] will do what I attempted to do in my example, and either std::unique_ptr or std::shared_ptr will take care of destruction.

The best advice? Read about RAII (and design according to it), use the Standard Library, don't create naked resources, and get familiarized with what Herb Sutter has to say with respect to "exception safety" (go ahead and read his website, or google "Herb Sutter Exception Safety")

Answer 2

Avoid the need to allocate memory on the heap (via new and new[]) by using standard library containers. If this is not possible, always use a smart pointer, like std::unique_ptr<> to manage memory allocated on the heap. Then you will never need to write code for deleting the memory and it will be automatically cleaned up even in case an exception is thrown in your constructor (actually a constructor is often a likely place for an exception, but a destructor should really not throw).

Answer 3

If you must handle a resource, and your use case is not handled by any of the utilities in the standard library, then the rule is simple. Handle one, and only one resource. Any class that needs two resources handled should store two objects that are capable of handling themselves (i.e. objects that follow RAII). As a simple example of what not to do, lets say you wanted to write a class that needed a dynamic array of ints, and a dynamic array of doubles, (forget the standard library for the moment). What you would not do is this:

class Dingbat
{
public:
    Dingbat(int s1, int s2)
    {
        size1 = s1;
        size2 = s2;
        a1 = new int[s1];
        a2 = new int[s2];
    }
    ...
private:
    int * a1;
    double * a2;
    int size1, size2;
};

The problem with the above constructor is that if the allocation for a2 fails, an exception is thrown, and the memory for a1 is not released. You could of course handle this with try catch blocks, but it becomes much more complex (needlessly) when you have multiple resources.

Instead, you should write classes (or a single class template in this case) that handles a single dynamic array properly, taking care of initializing itself, copying itself and disposing of itself. If there is only one call to new, then you don't need to worry if the allocation fails. An exception will be thrown and no memory needs to be released. (you may want to handle it anyway and throw your own custom exception in order to be more informative)

Once you have that/those classes all finished, then your Dingbat class would include each of these objects. The Dingbat class is then much simpler, and probably doesn't need any special routines for handling initialization, copying or destruction.

This example is, of course, hypothetical, as the above situation is handled already by std::vector. But like I said, this is for if you happen to have a situation that isn't covered by the standard library.

Answer 4

What you can very often do is call the function that can fail before the constructor, and call the instructor with the value that the function that could fail returns.

#include <string>
#include <iostream>
#include <memory>

class Object {};

This is just some Object that our class needs. It could be a connected socket, or a bound socket. Something that can fail when it is trying to connect or bind inside a constructor.

Object only_odd( int value ) {
    if ( value % 2 == 0 )
        throw "Please use a std::exception derived exception here";
    else
        return Object();
}

This function returns a object and throws when it fails (for every even number). So this could be what we first would have like to done in the destructor.

class ugly {
    public:
        ugly ( int i ) {
            obj = new Object;
            try{
                *obj = only_odd( i );
            }
            catch ( ...) {
                delete obj;
                throw ( "this is why this is ugly" );
            }
        }

        ~ugly(){ delete obj; }

    private:

        Object* obj;
};

better takes the pre constructed value that might fail and therefore throw. Therefore we could also construct the better class from an already initialised object. Then we can do error handling even before the class gets constructed and then we do not have to throw from the constructor. And even better, it uses smart pointers for memory handling, that way we can be really pretty sure the memory gets deleted.

class better {

    public:

        better ( const Object& org ) : obj { std::make_shared<Object>(org) }
        {
        }

    private:
        /*Shared pointer will take care of destruction.*/
        std::shared_ptr<Object>  obj;
};

and this might be how we would use it.

int main ( ) {
    ugly (1);

    /*if only odd where to fail it would fail allready here*/
    Object obj = only_odd(3); 
    better b(obj);

    try { /*will fail since 4 is even.*/
        ugly ( 4  );
    }
    catch ( const char* error ) {
        std::cout << error << std::endl;
    }
}