Avoiding indirect cyclic references when using shared_ptr and weak_ptr

Question

I'm currently putting together an application that relies heavily on shared_ptr and everything looks good so far - I've done my homework and have a pretty good idea of some of the pitfalls of using shared_ptrs.

One of the most recognised problems with shared_ptr is cyclic dependencies - these issues can be solved by storing weak_ptrs that don't affect the lifetime of objects up the chain. However, I'm struggling to get my head around times where it's necessary to store a pointer to an external object via a weak_ptr - I'm not sure whether it's forbidden, discouraged, or whether it's safe.

The following diagram describes what I mean (black arrows indicate shared_ptr; dashed indicate weak_ptr):

alt text http://img694.imageshack.us/img694/6628/sharedweakptr.png

• A parent contains shared_ptrs to two children, both of which point back to the parent using a weak_ptr.
• In the constructor of the first child I retrieve via the parent weak_ptr the pointer to the second child and store it locally.

The code looks like this:

#include <boost/shared_ptr.hpp>
#include <boost/weak_ptr.hpp>
#include <boost/make_shared.hpp>
#include <boost/enable_shared_from_this.hpp>

class child;
class child2;
class parent;

class parent : public boost::enable_shared_from_this<parent>
{
public:
    void createChildren()
    {
        _child2 = boost::make_shared<child2>(shared_from_this());
        _child = boost::make_shared<child>(shared_from_this());
    }

    boost::shared_ptr<child> _child;
    boost::shared_ptr<child2> _child2;
};

class child
{
public:
    child(boost::weak_ptr<parent> p)
    {
        _parent = p;
        _child2 = boost::shared_ptr<parent>(p)->_child2; // is this safe?
    }

    boost::weak_ptr<parent> _parent;
    boost::shared_ptr<child2> _child2;
};

class child2
{
public:
    child2(boost::weak_ptr<parent> p)
    {
        this->_parent = p;
    }

    boost::weak_ptr<parent> _parent;
};

int main()
{
    boost::shared_ptr<parent> master(boost::make_shared<parent>());
    master->createChildren();
}

I've tested this and it seems to work ok (I don't get any reports of memory leaks), however my question is: Is this safe? And if not, why not?

Answer 1

The child constructor appears to be safe the way you are calling it. However its not safe in general.

The issue is due to passing in a weak_ptr as the argument in the child constructor. This means you need to worry about whether the weak pointer is for an object that no-longer exists. By changing this parameter to a shared_ptrs and converting to a weak_ptr when storing we know that the object still exists. Here's the change:

child(boost::shared_ptr<parent> p)
{
    _parent = p;
    _child2 = p->_child2; // This is this safe
}

Answer 2

One of the most recognised problems with shared_ptr is cyclic dependencies - these issues can be solved by storing weak_ptrs that don't affect the lifetime of objects up the chain.

Wrong. This cyclic dependency exists or it does not.

If the issue exists, then weak reference is simply not an option.

where it's necessary to store a pointer to an external object via a weak_ptr

weak_ptr is almost never needed.

There are few quite specific cases where weak_ptr is appropriate, but mostly it's part of the shared_ptr cult: instead of randomly throwing shared_ptr at problems, they randomly throw half shared_ptr and half weak_ptr (as seen on SO).

Answer 3

You'll get bad_weak_ptr exception if 'p' was (somehow) destroyed already. So it is safe is child ctor expects exceptions and not safe otherwise.