Why does the destructor call itself endlessly (causing a stack overflow)?

Question

I am confused on why the destructor calls itself an endless amount of times, when I try to construct an object say LeakySingleton on the heap through a static function call create_instance() and then try to delete it explicitly afterwards via the delete operation.

As I understand it, considering the source listings from below, the variable leaky_singleton inside main() gets to point to the heap allocated resource returned by create_instance(). Thus, we have allocated an object LeakySingleton on the heap indirectly through the create_instance function. Now, if I explicitly call the delete operator or delete function on leaky_singleton, then it calls the destructor first and checks whether or not it satisfies the instance != nullptr condition and then the deletion of the object that instance points to should be deleted. If this object LeakySingleton::instance is deleted, then there is no reason for the dtor to call itself again, or am I missing something here?

Calling it with and without valgrind leads to a segmentation fault (invalid memory access due to stack overflow):

Segmentation fault (core dumped)

Stepping through with the debugger, leads to endless destructor calls (the culprit of the stack overflow).

From cplusplus.com (http://www.cplusplus.com/forum/general/40044/):

If you delete your object, it attempts to delete itself, which will cause it to attempt to delete itself, which will cause it to delete itself, which will...

Why does it attempt to delete itself, when I simply use the delete operator/function to deallocate the heap object LeakySingleton pointed to by the static class member variable LeakySingleton::instance? The heap allocated resource is pointed to by the LeakySingleton::instance pointer variable which points to a LeakySingleton object. So why does an explicit delete function call doesn't delete or rather deallocate the allocated heap object and instead recurses endlessly? What am I missing here?

(My current dtor and ctor understanding: The new function/operator allocates memory for an object on the heap and calls the constructor, and the delete function calls the destructor and in my case also calls a delete operator/function inside.)

Source:

main.cpp:

class Singleton final
{
    public:
        static Singleton & create_instance(int);
        ~Singleton() = default;
    private:
        int x;
        Singleton(int);

        Singleton(Singleton &) = delete;
        Singleton(Singleton &&) = delete;
        Singleton & operator=(Singleton &) = delete;
        Singleton & operator=(Singleton &&) = delete;
};

Singleton::Singleton(int t_x) : x{t_x}
{}

Singleton & Singleton::create_instance(int t_x)
{
    static Singleton instance{t_x};
    return instance;
}

// Potential endless dtor calls inside:
class LeakySingleton final
{
    public:
        static LeakySingleton * create_instance(int);
        ~LeakySingleton();
    private:
        int x;
        static LeakySingleton * instance;
        LeakySingleton(int);

        LeakySingleton(LeakySingleton &) = delete;
        LeakySingleton(LeakySingleton &&) = delete;
        LeakySingleton & operator=(LeakySingleton &) = delete;
        LeakySingleton & operator=(LeakySingleton &&) = delete;
};

LeakySingleton * LeakySingleton::instance = nullptr;

LeakySingleton::LeakySingleton(int t_x) : x{t_x}
{}

LeakySingleton::~LeakySingleton()
{
    if (instance != nullptr)
    {
        delete instance;
        instance = nullptr;
    }
}

LeakySingleton * LeakySingleton::create_instance(int t_x)
{
    if (instance == nullptr)
    {
        instance = new LeakySingleton{t_x};
    }
    return instance;
}

int main()
{ 
    // The correct implementation with no issues:
    {
        Singleton & singleton = Singleton::create_instance(42);
    }

    // The faulty implementation causing the dtor to recurse endlessly and resulting in a segfault:
    {
        LeakySingleton * leaky_singleton = LeakySingleton::create_instance(42);
        delete leaky_singleton;
    }

    return 0;
}

Makefile:

CC = g++
CFLAGS = -g -Wall -Wextra -pedantic -std=c++11
SRC = main.cpp
TARGET = app
RM = rm -rf

.PHONY: all clean

all: $(TARGET)

clean:
    $(RM) $(TARGET)

$(TARGET): $(SRC)
    $(CC) $(CFLAGS) $^ -o $@

Answer 1

You have a nasty cycle, in LeakySingleton::create_instance you have:

instance = new LeakySingleton{t_x};

then in LeakySingleton's destuctor you have:

delete instance;

which is going to call LeakySingleton's destuctor before you set anything to null:

instance = nullptr;

so you have infinte recursion causing your stackoverflow.

Answer 2

In C++, delete will call the class destructor.

The delete statement in your main function is calling LeakySingleton::~LeakySingleton, which in turn tries to delete the static instance pointer, which then calls the destructor again. Your code never had the chance to set the static pointer to null. There you have an infinite loop.

P.S. IMHO, it is generally a bad practice to modify static members in non-static methods. I believe you can put the static clean up logic in another static method.

class LeakySingleton final {
public:
  static LeakySingleton& create_instance(int);
  static void destroy_instance();
  ~LeakySinglton() = default;
private:
  static LeakySingleton *instance;
  ...
};

void LeakySingleton::destroy_instance() {
  if (instance != nullptr) {
    delete instance;
    instance = nullptr;
  }
}

Answer 3

Delete instace in destructor initiate a destructor call.

Answer 4

First since the LeakySingleton shall not be created directly, it shall not be destroyed directly either:

• Therefore its destructor should be private, exactly as its constructor.
• if the singleton may be deleted: it shall be deleted with a public function delete_instance() that deletes the instance
• the destructor shall not delete itself (endless rescursion)
• this construct should avoid this endless destructor recursion

If you want the instance pointer to leak and allow its destruction, you shall not do it twice (once outside and once inside the destructor), but only once outside the destructor. Since there is only one instance the destructor outside means that there is no need for a delete call inside:

LeakySingleton::~LeakySingleton()
{
    if (instance != nullptr)
    {
         instance = nullptr;  // since there's only one, it's the instance and
    }                         // the instance pointer shall be reset
    // and do what's needed to clean the object
}   

Note: this implementation is not thread-safe.

Note 2: this article could be of interest to you. It also warns against public destructor, since this could lead to dangling pointers.