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?

Serafina

try/catch is not code smell, and is often underused IMO. – Gumwood

look here: parashift.com/c++-faq-lite/selfcleaning-members.html – Kalagher

@MooingDuck: Indeed, by themselves they don't smell. But try {} catch(...) {throw;} has rather a strong odour. – Hoiden

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()).

Hoiden

What I was going to say. I'd add that once you've written a class to manage each resource, you can aggregate several of them together as data members of another class, if the combination of resources makes sense. If a data member constructor throws, then any members already initialized are destroyed. – Appetizing

Yes, but The acquistion of the resource involves several steps: DoSomething is the first step in acquiring the resource. – Serafina

@Roddy: ITYM, "there are several resources to acquire". You might not realise yet that they're separate resources, but the RAII pattern is doing its best to tell you :-) – Appetizing

@Steve, OK, so DoSomething/UndoSomething is the 'first' resource - that seems to imply it should have a separate RAII object to manage it - like C.Stoll suggests. – Serafina

Re. Rule of Three. BOOST_NONCOPYABLE was omitted from sample for clarity... ;-) – Serafina

@MikeSeymour: "The principle of RAII is that the constructor acquires a resource". The constructor does not necessarily acquire the resource. – Mercier

@Nawaz: OK, I've expanded my over-simplification. – Hoiden

@MikeSeymour: That looks better. :-).. +1 – Mercier

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
  }
} 

Lallygag

@Nick it makes compilation fail if that function is selected by overload resolution. It's a new feature. You can achieve something similar in compilers that don't support this feature by making it private. – Lallygag

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.

Denude

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.

Heterogynous

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.

Gumwood

Thanks. Agree re. exceptions - but I've seen so much cleanup code in catch statements that I still treat them with suspicion. The concept was that the cleanup function is 'optional' to avoid calling UndoSomething if there's an exception thrown before DoSomething was called. void Foo() : b(UndoSomething) { ... DoSomething()); – Serafina

@Roddy: If the cleanup is in the miniclass, the initialization should also be in the miniclass, making that a non-issue. – Gumwood

sounds like void Foo() : b(DoSomething, UndoSomething) is required. Interesting... – Serafina

Or better yet, have those be members of b, instead of passing function pointers around, such as all of the other answers suggested. – Gumwood

Yes, that's probably the best way. This pattern crops up a lot when interfacing to non C++ code, and I was considering having a single class that took 'do' and 'undo' parameters as boost::function types (or is it boost::bind - never sure) to avoid having to code lots of these little classes. – Serafina

The little classes make it almost impossible to make mistakes, and the code runs faster. That's the core of C++. – Gumwood

I just stumbled on Alexandrescu's ScopeGuard: It's close to my idea for a cleanup function. #49147 – Serafina

@Roddy: That's also for situations where the variables are locals, not members. I see nothing in your question that implies you have the problem he's trying to solve. – Gumwood

It was an idea to avoid the "lots of these little classes" I mentioned earlier. The scopeguard article mentions (briefly) about a Janitor class which is suitable as a member. – Serafina

Answer 6

Rules of thumb:

• If your class is manually managing the creation and deletion of something, it is doing too much.
• If your class has manually written copy-assignment/-construction, it is probably managing too much
• Exception to this: A class that has the sole purpose of managing exactly one entity

Examples for the third rule are std::shared_ptr, std::unique_ptr, scope_guard, std::vector<>, std::list<>, scoped_lock, and of course the Trasher class below.

---

Addendum.

You could go so far and write something to interact with C-style stuff:

#include <functional>
#include <iostream>
#include <stdexcept>


class Trasher {
public:
    Trasher (std::function<void()> init, std::function<void()> deleter)
    : deleter_(deleter)
    {
        init();
    }

    ~Trasher ()
    {
        deleter_();
    }

    // non-copyable
    Trasher& operator= (Trasher const&) = delete;
    Trasher (Trasher const&) = delete;

private:
    std::function<void()> deleter_;
};

class Foo {
public:
    Foo ()
    : meh_([](){std::cout << "hello!" << std::endl;},
           [](){std::cout << "bye!"   << std::endl;})
    , moo_([](){std::cout << "be or not" << std::endl;},
           [](){std::cout << "is the question"   << std::endl;})
    {
        std::cout << "Fooborn." << std::endl;
        throw std::runtime_error("oh oh");
    }

    ~Foo() {
        std::cout << "Foo in agony." << std::endl;
    }

private:
    Trasher meh_, moo_;
};

int main () {
    try {
        Foo foo;
    } catch(std::exception &e) {
        std::cerr << "error:" << e.what() << std::endl;
    }
}

output:

hello!
be or not
Fooborn.
is the question
bye!
error:oh oh

So, ~Foo() never runs, but your init/delete pair is.

One nice thing is: If your init-function itself throws, your delete-function won't be called, as any exception thrown by the init-function goes straight through Trasher() and therefore ~Trasher() won't be executed.

Note: It is important that there is an outermost try/catch, otherwise, stack unwinding is not required by the standard.

Throve