why am i getting this runtime exception when using shared_ptr?

Question

In the following code, I am getting the following runtime exception (possibly memory leak) after return 1; and in destructor of Node().

Unhandled exception at 0x0f9bad4a (msvcp100d.dll) in test.exe: 0xC0000005: Access violation reading location 0xfeeefef2.

It's been a while since I used smart_ptr, so am trying to learn what am I doing wrong here ?

#include <vector>
#include <queue>
#include <memory>

#include <iostream>
using namespace std;

class Node;
typedef shared_ptr<Node> SharedNode;

class Node {
    Node* parent;
    vector< SharedNode > children;
    int value;

    //limiting construction
    Node(int a_value):value(a_value),parent(0){}
    Node(const Node &copy); //non-construction-copyable
    Node& operator=(const Node& copy); //non-copyable
public:
    static SharedNode create(int a_value){
        return SharedNode(new Node(a_value));
    }
    SharedNode addChild(SharedNode child){
        child->parent = this;
        children.push_back(child);
        return child;
    }

SharedNode getNode(int searchValue);
};

SharedNode Node::getNode(int searchValue){

    // Breadth First Search
    queue<SharedNode> que;
    que.push(SharedNode(this));

    while(!que.empty()){
        SharedNode node = que.front();
        que.pop();

        if(node->value == searchValue)
            return node;

        vector<SharedNode>::iterator it;
        for(it = node->children.begin(); it != node->children.end(); it++){
            que.push(*it);
        }
    }

    return 0;
}

int main(){
    SharedNode node_ptr = Node::create(5);

    for(int i  = 0; i < 4; ++i)
        node_ptr->addChild(Node::create(i));

    cout << (node_ptr->getNode(-1) != 0 ? "Found" : "Not found");

    return 1;
}

I think I'm messing up when I use shared_ptr on this, like: shared_ptr(this). But then, that's my guess.

What am I doing wrong here ?

Answer 1

The problem is from

que.push(SharedNode(this));

This creates a new shared pointer that now owns this. However, due to the create() method, there is another shared pointer that owns the same object. This can result in a double delete.

If you have a reason to use a shared pointer in this situation, the correct solution is enable_shared_from_this.

First, change the node definition to this.

class Node : public std::enable_shared_from_this<Node> { ...

Then change the offending line to

que.push(this->shared_from_this());

This causes it to return a shared_ptr that points to the object, but it is shared with the already existing shared_ptr, instead of being two separate shared_ptr objects.

Note, for the use of this->shared_from_this() to be legal, the object must be owned by a shared_ptr. You already have accomplished this via the static create() method, but I wanted to make sure you understood the limitation.

Edit: A brief explanation of shared_ptr ownership.

When you create a shared_ptr from a raw pointer using the constructor, it creates a reference object that contains both a pointer to the object and a reference count, which is used to determine how many shared_ptr objects point to it. A pointer to this reference object is then passed to all copies that are made from that original shared_ptr, with the reference count keeping track of how many shared_ptr objects refer to it.

When you call shared_ptr(this), there is no way for the shared pointer to know that this is owned by another shared pointer, and creates a new reference object. Once the one of them reaches a reference count of zero, the object will be deleted, despite the other shared_ptr reference object still pointing to it, resulting in a dangling pointer and the error you are seeing.

If you only need the children to exist when the parent exists, I would consider changing the Node to simply have a std::vector of other Nodes (remove the pointer). When the highest level node is destroyed via its destructor, it will destroy the vector, which destroys the children nodes, and so-on.

class Node
{
  // whatever operations you need... 

  std::vector<Node> children;
}

Edit: As requested...

If you have a use case where you do really want to have the children outlive the parents, you'll have to deal with the parent pointer, since it could be destroyed before the children. One quick solution is determine if you really NEED the parent pointer, and eliminate it if you don't need it.

However, assuming you still want to retain it, you cannot use shared_ptr here. If you do that, you'll have a circular dependency, and neither will be destroyed automatically, which isn't what you want.

The solution here is to use std::weak_ptr. Basically, it interacts with the shared_ptr reference object in such a way that it doesn't prevent the destruction of the pointed to object.

class Node
{
private:
   std::weak_ptr<Node> parent;
   // Other constructors.  
   Node(int a_value):value(a_value),parent() {} 
public:
   SharedNode addChild(SharedNode child){
        child->parent = this->shared_from_this(); // Initialize their pointer using
                                                  // your shared pointer
        children.push_back(child);
        return child;
   }
   // This function will return a shared_ptr to nullptr (and will evaluate false) 
   // if you have no parent, or if the parent node has been deleted
   SharedNode getParent()
   {
       return parent.lock();
   }
};

Answer 2

Consider what happens with the following code:

Node * dumb_ptr = new Node;
shared_ptr<Node> smart1 = dumb_ptr;
shared_ptr<Node> smart2 = dumb_ptr;

You now have two smart pointers both thinking they own the same object. One of them is going to delete the object, and the other one will try to use or delete that deleted object at some point. The way you fix this is by always creating a smart pointer from another smart pointer or from new. Best to never use any dumb pointers at all - that includes this.

shared_ptr<Node> smart1 = new Node;
shared_ptr<Node> smart2 = smart1;