C++ cyclic destructors

Question

in file a.h:

class B;
class A
{
public:
    B *b;
    A(B *b = nullptr)
    {
        this->b = b;
    }
    ~A()
    {
        delete this->b;
    }
};

in file b.h:

class A;
class B   
{
public:
    A *a;
    B(A *a = nullptr)
    {
        this->a = a;
    }
    ~B()
    {
        delete this->a;
    };
};

lets pretend that we have a pointer to an A *object, and we want to delete it:

// ...
A *a = new A();
B *b = new B();
A->b = b;
B->a = a;
// ...

delete a;

// ...

A's deconstructor will say delete B; i.e. call B's deconstructor. B's deconstructor will say delete A. death loop lé infinitè.

Is there a better way to write code to solve this problem? This isn't pressing question, just curious.

Thanks!

Answer 1

Use smart pointers (i.e. std::shared_ptr) and weak pointers (i.e. std::weak_ptr) instead of pure pointers.

You have to define which class really owns which one. If none is owning the other, both should be weak pointers.

Answer 2

This code has undefined behavior. The first delete a happens, and then A::~A calls delete b. Then B::~B calls delete a, which results in a double free.

Your implementation is allowed to do whatever it wants in such cases, including infinite loop, or just "seem to work". You must ensure that objects are delete ed exactly once.

For this particular application, you might want to consider having A "own" B or vice versa. That way, e.g. A is always responsible for calling delete B. This common in cases where you have something of a tree of objects where children can reference parents and vice versa. In such cases, the parent "owns" the children and takes responsibility for deleting the children.

If you can't do that, you might be able to come up with some solution with shared_ptr and weak_ptr (this is cyclic, so shared_ptr alone doesn't help you); but you should strongly consider designing such that this situation does not arise.

In this specific example, the class A does not own the pointer to B -- main owns the pointer to B. Therefore, the destructor code of A should probably not delete anything -- that should be handled by main (or rather, hopefully std::unique_ptr instances inside of main).

Answer 3

This is a problem of circular data dependency, not a circular destructor dependency. If the chain of pointers a->b->a->b->...->a... eventually lead to a NULL, the destructor is going to terminate; if the trail of pointers leads back to the beginning, you will get undefined behavior on deleting the same object twice.

The problem is not much different from deleting a circular linked list: you can come back full circle if you are not careful. You can use a common technique commonly known as tortoise and hare to detect A/B loops in your structure, and set the "back pointer" to NULL before triggering the chain of destructors.

Answer 4

Here's a simple solution: Break any cycles by setting the field to null before you delete the object.

Edit: this won't work in general, for example the following will still crash:

A* a1 = new A();
B* b1 = new B();
A* a2 = new A();
B* b2 = new B();

a1->b = b1;
b1->a = a2;
a2->b = b2;
b2->a = a1;

delete a1;

-

class A
{
    ...
    ~A()
    {
        // does b points to us?

        if(this->b && this->b->a == this)
        {
            this->b->a = nullptr;

            // b no longer points to us
        }

        // can safely delete b now

        delete this->b;
    }
};

...

class B
{
    ...
    ~B()
    {
        // does a point to us?

        if(this->a && this->a->b == this)
        {
            this->a->b = nullptr;

            // a no longer points to us
        }

        // can safely delete a now

        delete this->a;
    }
};

Answer 5

First of all, you have a bad design here. If A and B are interlinked, and taking out an A destroys the B it links to and vice versa, there must be a way to refactor that relationship to a parent-child sort of relationship. A should be a "parent" to B if possible, so destroying a B doesn't destroy the A it links to, but destroying an A destroys the B it contains.

Of course things don't always work out that way and sometimes you have this unavoidable cyclic dependency. To make it work (until you have a chance to refactor!) break the cycle on destruction.

struct A
{
  B* b;
  ~A(){
    if( b ) {
      b->a = 0 ; // don't "boomerang" and let ~B call my dtor again
      delete b ;
    }
  }
} ;

struct B
{
  A* a;
  ~B(){
    if( a ) {
      a->b = 0 ; // don't "boomerang" and let ~A call my dtor again
      delete a ;
    }
  }
} ;

Note this only works for the scenario where A and B point to each other

It does not apply to this situation

That above situation is a circularly linked list, and you'd have to look forward this->b->a->b until you reach this again, set that final pointer to NULL, then start the destruction process. This would be difficult to do unless A and B inherited from a common base class.