Exception safety:

Question

I would like to ask for some advice with regard to exception safety. In particular I have been referencing Do you (really) write exception safe code?. If I have a container of pointers to objects of type Node, and I were to clear and reinitialise that container of objects _nodes with a new collection of objects, would this code be exception safe?

std::vector<Node*> nodes;

for (int i = 0; i < 10; i++)
{
    try
    {
        // New can throw an exception. We want to make sure that if an exception is thrown any allocated memory is deleted.
        std::unique_ptr<Node> node(new Node());
        Node* n = node.get();
        nodes.push_back(n);
        node.release();
    }
    catch (std::exception& exception)
    {
        // If an exception is thrown, rollback new allocations and rethrow the exception.
        for (std::vector<Node*>::iterator it = nodes.begin(); it < nodes.end(); it++)
        {
            delete *it;
        }

        nodes.clear();
        throw exception;
    }

}

_nodes.swap(nodes);

// Delete the unused (previous) objects from the swapped container.
for (std::vector<Node*>::iterator it = nodes.begin(); it < nodes.end(); it++)
{
    delete *it;
}

I have also been reading into RAII, but I don't know how this would work where I need polymorphism (http://en.wikipedia.org/wiki/Polymorphism_(computer_science)#Subtyping).

Why are you doing a rollback when there is no indication that anything was pushed onto the vector? What if new() threw an exception, or possibly your Node constructor throws one (if it is possible it can throw)? — PaulMcKenzie, Mar 16 '14 at 19:58
@PaulMcKenzie could it not happen at some iteration other than the first. The constructor could throw a std::bad_alloc exception. — , Mar 16 '14 at 19:59
I don't understand the reason for your catch block to do what it's doing. If an exception is thrown, no item was added to the vector, but your catch block assumes that this "bad item" was added to the vector. — PaulMcKenzie, Mar 16 '14 at 20:15
@PaulMcKenzie that vector is declared outside of the loop. So it could throw an exception on the second iteration, but you would still have objects in the vector. — , Mar 16 '14 at 20:17

Jonathan Wakely · Answer 1 · 2014-03-16T20:30:22.787

That is a lot more complicated than it needs to be. I would start like this:

std::vector<std::unique_ptr<Node>> nodes(10);    
for (auto& p : nodes)
    p.reset(new Node());

If constructing the vector or allocating a Node throws then everything will be cleaned up automatically.

Now if you're sensible and replace _nodes with std::vector<std::unique_ptr<Node>> then the rest of the function is simply:

_nodes.swap(nodes);

Otherwise it's not quite as simple:

std::vector<Node*> nodes2;
nodes2.reserve(nodes.size());
for (auto p : nodes)
    nodes2.push_back(p.release());
_nodes.swap(nodes2);
for (auto p : nodes2)
    delete p;

Assuming the Node destructor can't throw, the only step here that can throw is the reserve call so if that throws nodes is cleaned up because it holds unique_ptr objects. After that you can safely transfer ownership to nodes2, then swap, then clean up.

I have also been reading into RAII, but I don't know how this would work where I need polymorphism

My code above relies on RAII, polymorphism is irrelevant. There are no raw pointers in the code above which are not owned by an RAII type (except in _nodes, which you should change to be std::vector<std::unique_ptr<Node>>) so if an exception is thrown everything gets cleaned up, there is no need to catch the exception and do manual cleanup.

Exception safety:

1 Answers1