Help me make this code exception-safe.

Question

So I have this library code, see...

class Thing
{
public:

    class Obj 
    {
     public:
        static const int len = 16;

        explicit Obj(char *str)
        {
            strncpy(str_, str, len);
        }

        virtual void operator()() = 0;

    private:
        char str_[len];
    };

    explicit Thing(vector<Obj*> &objs) : objs_(objs) {}

    ~Thing() {
        for(vector<Obj*>::iterator i = objs_.begin(); i != objs_.end(); ++i) {
            delete *i;
        }
    }

private:
    vector<Obj*> objs_;
}

And in my client code...

   class FooObj : public Thing::Obj
    {
        virtual void operator()() {
            //do stuff
        }
    }

    class BarObj : public Thing::Obj
    {
        virtual void operator()() {
            //do different stuff
        }
    }

vector<Objs*> objs;
int nStructs = system_call(*structs);
for(int i = 0; i < nStructs; i++) {
    objs.push_back(newFooObj(structs[i].str));
}
objs.push_back(newBarObj("bar1");
objs.push_back(newBarObj("bar2");

Thing thing(objs);
// thing does stuff, including call Obj::()() on the elements of the objs_ vector

The thing I'm stuck on is exception safety. As it stands, if any of the Obj constructors throw, or the Thing constructor throws, the Objs already in the vector will leak. The vector needs to contain pointers to Objs because they're being used polymorphically. And, I need to handle any exceptions here, because this is being invoked from an older codebase that is exception-unaware.

As I see it, my options are:

1. Wrap the client code in a giant try block, and clean up the vector in the catch block.
2. Put try blocks around all of the allocations, the catch blocks of which call a common cleanup function.
3. Some clever RAII-based idea that I haven't thought of yet.
4. Punt. Realistically, if the constructors throw, the application is about to go down in flames anyway, but I'd like to handle this more gracefully.

Answer 1

Instead of a vector of pointers to Obj you always can use a vector of Obj. In that case you need to make sure you can copy Obj safely (dangerous when it contains pointers). As your Obj contains only a fixed size char array it should be safe, though.

Answer 2

Answer 3 - Use smart pointers instead of Obj* in your vectors. I'd suggest boost::shared_ptr.

Answer 3

Since your Thing destructor already knows how to clean up the vector, you're most of the way towards a RAII solution. Instead of creating the vector of Objs, and then passing it to Thing's constructor, you could initialize Thing with an empty vector and add a member function to add new Objs, by pointer, to the vector.

This way, if an Obj's constructor throws, the compiler will automatically invoke Thing's destructor, properly destroying any Objs that were already allocated.

Thing's constructor becomes a no-op:

explicit Thing() {}

Add a push_back member:

void push_back(Obj *new_obj) { objs_.push_back(new_obj); }

Then the allocation code in your client becomes:

Thing thing(objs);
int nStructs = system_call(*structs);
for(int i = 0; i < nStructs; i++) {
    thing.push_back(newFooObj(structs[i].str));
}
thing.push_back(newBarObj("bar1");
thing.push_back(newBarObj("bar2");

As another poster suggested, a smart pointer type inside your vector would also work well. Just don't use STL's auto_ptr; it doesn't follow normal copy semantics and is therefore unsuitable for use in STL containers. The shared_ptr provided by Boost and the forthcoming C++0x would be fine.

Answer 4

Vector of Obj can be very poor for performance, since a vector could have to move object on resize and has to copy them all. Pointers to objects are better.

I've used Pointainers which will do what you need. Here is the original code.

/*
 * pointainer - auto-cleaning container of pointers
 *
 * Example usage:
 * {
 *     pointainer< std::vector<int*> > v;
 *     // v can be manipulated like any std::vector<int*>.
 *
 *     v.push_back(new int(42));
 *     v.push_back(new int(17));
 *     // v now owns the allocated int-s
 *
 *     v.erase(v.begin());
 *     // frees the memory allocated for the int 42, and then removes the
 *     // first element of v.
 * }
 * // v's destructor is called, and it frees the memory allocated for
 * // the int 17.
 *
 * Notes:
 * 1. Assumes all elements are unique (you don't have two elements
 *    pointing to the same object, otherwise you might delete it twice).
 * 2. Not usable with pair associative containers (map and multimap).
 * 3. For ANSI-challenged compilers, you may want to #define
 *    NO_MEMBER_TEMPLATES.
 *
 * Written 10-Jan-1999 by Yonat Sharon <yonat@@ootips.org>
 * Last updated 07-Feb-1999
 *
 * Modified June 9, 2003 by Steve Fossen
 *  -- to fix g++ compiling problem with base class typenames
 */

#ifndef POINTAINER_H
#define POINTAINER_H

#ifdef NO_MEMBER_TEMPLATES
    #include <functional> // for binder2nd
#endif

template <typename Cnt>
class pointainer : public Cnt
{
public:
    // sf - change to fix g++ compiletime errors
#ifdef USE_USING_NOT_TYPEDEF
    // I get compile errors with this
    using typename Cnt::size_type;
    using typename Cnt::difference_type;
    using typename Cnt::reference;
    using typename Cnt::const_reference;
    using typename Cnt::value_type;
    using typename Cnt::iterator;
    using typename Cnt::const_iterator;
    using typename Cnt::reverse_iterator;
    using typename Cnt::const_reverse_iterator;
#else
    // this way works
    typedef typename Cnt::size_type                 size_type;
    typedef typename Cnt::difference_type           difference_type;
    typedef typename Cnt::reference                 reference;
    typedef typename Cnt::const_reference           const_reference;
    typedef typename Cnt::value_type                value_type;
    typedef typename Cnt::iterator                  iterator;
    typedef typename Cnt::const_iterator            const_iterator;
    typedef typename Cnt::reverse_iterator          reverse_iterator;
    typedef typename Cnt::const_reverse_iterator    const_reverse_iterator;
#endif

    typedef pointainer< Cnt > its_type;

    pointainer()                            {}
    pointainer( const Cnt &c )   : Cnt( c ) {}

    its_type &operator=( const Cnt &c )      { Cnt::operator=( c ); return *this;        }
    ~pointainer()                            { clean_all();                              }

    void clear()                             { clean_all();   Cnt::clear();              }
    iterator erase( iterator i )             { clean( i );    return Cnt::erase( i );    }
    iterator erase( iterator f, iterator l ) { clean( f, l ); return Cnt::erase( f, l ); }

    // for associative containers: erase() a value
    size_type erase( const value_type& v )
    {
        iterator i = find( v );
        size_type found( i != end() ); // can't have more than 1
        if( found )
            erase( i );
        return found;
    }

    // for sequence containers: pop_front(), pop_back(), resize() and assign()
    void pop_front()    { clean( begin() ); Cnt::pop_front();                 }
    void pop_back()     { iterator i( end() ); clean( --i ); Cnt::pop_back(); }

    void resize( size_type s, value_type c = value_type() )
    {
        if( s < size() )
            clean( begin() + s, end() );
        Cnt::resize( s, c );
    }

#ifndef NO_MEMBER_TEMPLATES
    template <class InIter> void assign(InIter f, InIter l)
#else
    void assign( iterator f, iterator l )
#endif
    {
        clean_all();
        Cnt::assign( f, l );
    }

#ifndef NO_MEMBER_TEMPLATES
    template <class Size, class T> void assign( Size n, const T& t = T() )
#else
    void assign( size_t n, const value_type& t = value_type() )
#endif
    {
        clean_all();
        Cnt::assign( n, t );
    }

    // for std::list: remove() and remove_if()
    void remove( const value_type& v )
    {
        clean( std::find( begin(), end(), v ));
        Cnt::remove( v );
    }

#ifndef NO_MEMBER_TEMPLATES
    template <class Pred>
#else
    typedef std::binder2nd< std::not_equal_to< value_type > > Pred;
#endif
    void remove_if(Pred pr)
    {
        for( iterator i = begin(); i != end(); ++i )
            if( pr( *i ))
                clean( i );

        Cnt::remove_if( pr );
    }

private:
    void clean( iterator i )                { delete *i; *i = 0;            } // sf add *i = NULL so double deletes don't fail
    void clean( iterator f, iterator l )    { while( f != l ) clean( f++ ); }
    void clean_all()                        { clean( begin(), end() );      }

    // we can't have two pointainers own the same objects:
    pointainer( const its_type& ) {}
    its_type& operator=( const its_type& ) { return  NULL;} // sf - return null to remove no return value warning..
};

#endif // POINTAINER_H

Answer 5

Take a look at boost::ptr_vector

Answer 6

Without changing the type stored in objs to a type that can be copied and manage Obj* itself, you need to add a try/catch block to cleanup objs if an exception is thrown. The easiest way would be to do this:

vector<Obj *> objs;

try {...}
catch (...) 
{
    // delete all objs here
    throw;
}

Although you'll want to clean up objs anyway if an exception isn't thrown also.

Answer 7

Hmmm. I really like Commodore Jaeger's idea; it neatly clears up some of the funny smells this code was giving me. I'm reluctant to bring in the Boost libraries at this point; this is a somewhat conservative codebase, and I'd rather bring it into the 21st century squirming and complaining than kicking and screaming.