0

I am trying to identify a nasty bug where an object spontaneously gets corrupted while being inside a map, and after some hours of debugging, I think I might not have fully grasped the idea of std::shared_ptr.

Here is the Context: Inside a method, I declare a std::shared_ptr and initialize it to point to a clone of the current object (created by new). Then - after some modifications to the object - I call the same method on that pointer (recursively).
Inside the next recursion of the method, the decision is made to insert this object into a std::unordered_map (which is a class-attribute, so it is available on all recursion levels).
This is some pseudo-code to illustrate what I mean:

class A
{
  ...
  void DoSomething(void); // the recursive function
  A* Clone(void) const { return new A(this); }  // the clone method
  ...
  static std::unordered_map<std::shared_ptr<A>,int> myMap{};
};

void A::DoSomething(void)
{
  ...
  if (condition) myMap.insert({this,5});  // in a deeper recursive call, condition is true
  ...
  std::shared_ptr<A> pA(Clone());   // make a copy
  pA->...  // modify it 
  pA->DoSomething();  // here is the recursive call
  ...
}

Problem: Sometimes, the object behind the pointer inside the std::unordered_map is destroyed, and it seems like this happens when the original std::shared_ptr goes out of scope.

My (tentative) understanding: Calling a method of the object the std::shared_ptr points to does not increase the reference count - inside the called method, I have access to this, which is the ptr the std::shared_ptr points to, but what I do with this in there is not affecting the original std::shared_ptr.
To verify this, I added code to make an extra clone into an extra std::shared_ptr, right at the moment of insertion into the map, and then everything works fine (just slower, and uses double the memory, which is both an issue - class A has a lot of complex data).

Question: Is my understanding correct? If not, how would I call a method of a std::shared_ptr so that the this inside the method is still the 'std::shared_ptr'? Or is this not possible, and I have to use another design?

Regarding duplicates: Should we pass a shared_ptr by reference or by value? seems to point that way, but is about passing parameters by value or by reference, which is not a choice I have with the this pointer.

Aganju
  • 5,994
  • 1
  • 9
  • 22
  • 5
    I'm not sure I fully understand you, but is [std::enable_shared_from_this](http://en.cppreference.com/w/cpp/memory/enable_shared_from_this) what you are looking for? – freakish May 09 '18 at 19:18
  • 1
    You insert `this` into the map, but `this` is owned by a `shared_ptr` (via the recursive call) when the owning `shared_ptr` (in the outer call `pA`) goes out of scope as the recursion unwinds the object in the map will be destroyed. – Richard Critten May 09 '18 at 19:25
  • Yes, @RichardCritten , but how do I keep it alive? The whole reason to use shared_ptr was that it stays alive. Looks like I should scrap the shared-ptr thing here, and use my own logic for when i delete it. – Aganju May 09 '18 at 19:27
  • The question is why have a shared_ptr at all? If the map is to own the object, either put the object itself (by value) into the map or put a `unique_ptr` in the map – Richard Critten May 09 '18 at 19:27
  • I learned to prefer shared and unique ptr to simple new and delete, it is recommended to avoid issues. So I tried to learn to use it. Probably not the right thing for this use case though. – Aganju May 09 '18 at 19:29
  • Why doesn't the map hold shared_ptr's if it's supposed to keep the objects alive as long as they are in the map? – Jesper Juhl May 09 '18 at 19:41

1 Answers1

2

Your understanding is basically correct. This line is your problem:

if (condition) myMap.insert({this,5});

Because of the raw this a completely independent shared_ptr with its own independent reference count is created in this line. Later in the outer recursion level at the end of DoSomething() the original shared_ptr pA goes out of scope, its refcount drops to 0, the object is destroyed and the second shared_ptr in the map starts to dangle.

Solution 1

You can solve it with std::enable_shared_from_this:

class A : public std::enable_shared_from_this<A> { ... }

// Btw: Lose the void pseudo-parameter. This is not C. ;)
void A::DoSomething()
{
    if (condition) {
        myMap.insert({shared_from_this(), 5});
    }
}

Potential Solution 2

From the code snippets you show I find it highly questionable that you need shared_ptr at all. Nothing of what you show indicates shared ownership. If that’s indeed the case, switch to unique_ptrs and std::move() them around. That gets rid of the problem, too.

besc
  • 2,282
  • 9
  • 10