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")