How to handle constructor failure for RAII

Question

I'm familiar with the advantages of RAII, but I recently tripped over a problem in code like this:

class Foo
{
  public:
  Foo()
  {
    DoSomething();
    ...     
  }

  ~Foo()
  {
    UndoSomething();
  } 
} 

All fine, except that code in the constructor ... section threw an exception, with the result that UndoSomething() never got called.

There are obvious ways of fixing that particular issue, like wrap ... in a try/catch block that then calls UndoSomething(), but a: that's duplicating code, and b: try/catch blocks are a code smell that I try and avoid by using RAII techniques. And, the code is likely to get worse and more error-prone if there are multiple Do/Undo pairs involved, and we have to clean up half-way through.

I'm wondering there's a better approach to doing this - maybe a separate object takes a function pointer, and invokes the function when it, in turn, is destructed?

class Bar 
{
  FuncPtr f;
  Bar() : f(NULL)
  {
  }

  ~Bar()
  {
    if (f != NULL)
      f();
  }
}   

I know that won't compile but it should show the principle. Foo then becomes...

class Foo
{
  Bar b;

  Foo()
  {
    DoSomething();
    b.f = UndoSomething; 
    ...     
  }
}

Note that foo now doesn't require a destructor. Does that sound like more trouble than it's worth, or is this already a common pattern with something useful in boost to handle the heavy lifting for me?

Answer 1

The problem is that your class is trying to do too much. The principle of RAII is that it acquires a resource (either in the constructor, or later), and the destructor releases it; the class exists solely to manage that resource.

In your case, anything other than DoSomething() and UndoSomething() should be the responsibility of the user of the class, not the class itself.

As Steve Jessop says in the comments: if you have multiple resources to acquire, then each should be managed by its own RAII object; and it might make sense to aggregate these as data members of another class that constructs each in turn. Then, if any acquisition fails, all the previously acquired resources will be released automatically by the individual class members' destructors.

(Also, remember the Rule of Three; your class needs to either prevent copying, or implement it in some sensible way, to prevent multiple calls to UndoSomething()).

Answer 2

Just make DoSomething/UndoSomething into a proper RAII handle:

struct SomethingHandle
{
  SomethingHandle()
  {
    DoSomething();
    // nothing else. Now the constructor is exception safe
  }

  SomethingHandle(SomethingHandle const&) = delete; // rule of three

  ~SomethingHandle()
  {
    UndoSomething();
  } 
} 


class Foo
{
  SomethingHandle something;
  public:
  Foo() : something() {  // all for free
      // rest of the code
  }
} 

Answer 3

I would solve this using RAII, too:

class Doer
{
  Doer()
  { DoSomething(); }
  ~Doer()
  { UndoSomething(); }
};
class Foo
{
  Doer doer;
public:
  Foo()
  {
    ...
  }
};

The doer is created before the ctor body starts and gets destroyed either when the destructor fails via an exception or when the object is destroyed normally.

Answer 4

You've got too much in your one class. Move DoSomething/UndoSomething to another class ('Something'), and have an object of that class as part of class Foo, thusly:

class Foo
{
  public:
  Foo()
  {
    ...     
  }

  ~Foo()
  {
  } 

  private:
  class Something {
    Something() { DoSomething(); }
    ~Something() { UndoSomething(); }
  };
  Something s;
} 

Now, DoSomething has been called by the time Foo's constructor is called, and if Foo's constructor throws, then UndoSomething gets properly called.

Answer 5

try/catch is not code smell in general, it should be used to handle errors. In your case though, it would be code smell, because it's not handling an error, merely cleaning up. Thats what destructors are for.

(1) If everything in the destructor should be called when the constructor fails, simply move it to a private cleanup function, which is called by the destructor, and the constructor in the case of failure. This seems to be what you've already done. Good job.

(2) A better idea is: If there's multiple do/undo pairs that can destruct seperately, they ought to be wrapped in their own little RAII class, which does it's minitask, and cleans up after itself. I dislike your current idea of giving it an optional cleanup pointer function, that's just confusing. The cleanup should always be paired with the initialization, that's the core concept of RAII.