How to implement proper stack unwinding and raii when thrown a bad_alloc during construction

Question

So I'm designing a class that's gonna be handling a bunch of memory and I want to make sure it unwinds properly if something goes wrong during memory allocation in it's constructor.

Here's what I've got:

class foo{
public:
    foo(): var1(nullptr), var2(nullptr){
        try{
        var1 = new int(1);
        var2 = new int(2);
        }
        catch(std::exception &e){
            delete var1;
            delete var2;
            return;
        }
    //Some other things involving these variables
}

Am I right in having this return in the catch block if I want to just want to stop constructing this object before some later code in the constructor causes an error because of the bad_alloc exception? Or does the catch block just cancel the constructor when it terminates? Am I overcomplicating the whole thing?

Search for `std::unique_ptr` and especially `std::make_unique` — Guillaume Racicot, May 19 '17 at 07:42

Richard Hodges · Answer 1 · 2017-05-19T08:28:26.323

I want to make sure it unwinds properly if something goes wrong during memory allocation in it's constructor.

Then you must make your code conform to the idea of "one class controls one resource or does one job"

#include <memory>

// foo has one job - to manage fooness. 
// It no longer manages memory resources

class foo
{
public:
    foo()
    : var1(new int(1))
    , var2(new int(2))
    {
    }

    std::unique_ptr<int> var1;   // unique_ptr only manages a resource
    std::unique_ptr<int> var2;   // unique_ptr only manages a resource
};

other correct initialisation forms:

#include <memory>

// foo has one job - to manage fooness. 
// It no longer manages memory resources

class foo
{
public:
    foo()
    : var1(std::make_unique<int>(1))
    , var2(std::make_unique<int>(2))
    {
    }

    std::unique_ptr<int> var1;   // unique_ptr only manages a resource
    std::unique_ptr<int> var2;   // unique_ptr only manages a resource
};

default values:

#include <memory>

// foo has one job - to manage fooness. 
// It no longer manages memory resources

class foo
{
public:
    // default constructor now implicit

    std::unique_ptr<int> var1 = std::make_unique<int>(1);   // unique_ptr only manages a resource
    std::unique_ptr<int> var2 = std::make_unique<int>(2);   // unique_ptr only manages a resource
};

Correct, but idiomatically unpleasant - redundant initialisation:

#include <memory>

// foo has one job - to manage fooness. 
// It no longer manages memory resources

class foo
{
public:
    foo()
    : var1(nullptr)
    , var2(nullptr)
    {
        var1 = std::make_unique<int>(1);
        var2 = std::make_unique<int>(2);
    }

    std::unique_ptr<int> var1;   // unique_ptr only manages a resource
    std::unique_ptr<int> var2;   // unique_ptr only manages a resource
};

Here's one way to do it without composition. Note all the extra complexity for no gain:

#include <memory>

// foo now has two jobs - to manage fooness, and to manage resources. 
// This adds un-necessary complication, bugs and maintenance overhead

class foo
{
public:
    foo()
    : var1(nullptr)
    , var2(nullptr)
    {
        var1 = new int(1);
        var2 = new int(2);
    }

    foo(const foo& other)
    : var1(nullptr)
    , var2(nullptr)
    {
        if (other.var1) {
            var1 = new int(*(other.var1));
        }

        if (other.var2) {
            var2 = new int(*(other.var2));
        }
    }

    foo(foo&& other) noexcept
    : var1(other.var1)
    , var2(other.var2)
    {
        other.var1 = nullptr;
        other.var2 = nullptr;
    }

    foo& operator=(const foo& other)
    {
        foo tmp(other);
        std::swap(var1, tmp.var1);
        std::swap(var2, tmp.var2);
        return *this;
    }

    foo& operator=(foo&& other) noexcept
    {
        foo tmp(std::move(other));
        std::swap(var1, tmp.var1);
        std::swap(var2, tmp.var2);
        return *this;
    }

    ~foo() noexcept
    {
        delete var1;
        delete var2;
    }

    int* var1;   // doesn't manage anything
    int* var2;   // doesn't manage anything
};

You should use ``make_unique`` (C++14) for the creation of the creation of the ``unique_ptr`` — nefas, May 19 '17 at 07:52
@nefas i didn't want to introduce two concepts at once. The new method is guaranteed by the standard to be correct, but you're right. I would use `make_unique` in my code. — Richard Hodges, May 19 '17 at 07:54
Yes but since the order of execution of the instruction can change, there is some case where using new to construct a ``unique_ptr`` can result in a leak. That's why ``make_unique`` was created. So IMHO it's better to "teach" both of them together. — nefas, May 19 '17 at 08:03
@nefas no, make_unique was created to provide symmetry with make_shared (which increases efficiency by coalescing the control block with the controlled object). The memory model guarantees that code reordering will never break the 'as if' rule. i.e. the output of the code will always be 'as if' the code you wrote was executed as written. This includes exception handling. — Richard Hodges, May 19 '17 at 08:07
@nefas however, some alternative initialisation forms added for completeness. — Richard Hodges, May 19 '17 at 08:14
In this case, there can be no harmful code re-ordering but there are some case where there can be: cf [this answer](http://stackoverflow.com/a/37514601/4534275). — nefas, May 19 '17 at 08:40
@nefas I completely take on board your point. The unspecified execution order of function argument evaluation argues for make_unique. However, a class initialisation list has a specified, linear execution order. So in this specific example it does not apply. — Richard Hodges, May 19 '17 at 08:46
Oh okay, I see. So instead of trying to do all the memory management by myself from scratch I should just use smart pointers and avoid having to worry about it in general. But, just for my own curiosity, would what I have (accompanied by a proper destructor for the foo class) be sufficient to stop memory leaks? And is it always a better idea to use smart pointers than bother with making my own memory manager? — wbrege, May 19 '17 at 19:47
@wbrege yes, since we have unique_ptr, shared_ptr and (in c++17 and boost) optional and variant, there is no need whatsoever to reinvent the wheel. All these structures manage memory for you safely and correctly, leaving you free to focus on implementing logic. — Richard Hodges, May 19 '17 at 19:50

score 0 · Answer 2 · answered May 19 '17 at 07:51

You should check out the smart pointers: std::unique_ptr (the one that would be useful in this case) and std::shared_ptr/std::weak_ptr (not the right type for your example but good to know).

As long as you are not re-implementing some kind of smart pointer, use them instead of dynamic allocation, it will save you some troubles (even if your example is correct and will not leak, provided that the destructor is correct).

Instead of returning, you should throw an exception: the caller should be notified that the construction failed (if the allocation failure means a failure of the constructor). RAII means that the object should never be in an invalid state.

How to implement proper stack unwinding and raii when thrown a bad_alloc during construction

2 Answers2