Singleton pattern destructor C++

Question

i have this singleton pattern and it runs ok. But when i execute my program with valgrind to check memory leaks, it seems that the instance is never destroyed.

Where is my mistake?

Header

class Stopwords {   
    private:
        static Stopwords* instance;
        std::map<std::string,short> diccionario;

    private: 
    Stopwords();

    public:
        ~Stopwords();

    public:
    static Stopwords* getInstance();
    std::map<std::string,short> getMap();
};

.cpp

Stopwords* Stopwords::instance = NULL;

Stopwords::Stopwords() {
    diccionario = map<string,short>();

    char nombre_archivo[] = "stopwords/stopwords.txt";
    ifstream archivo;
    archivo.open(nombre_archivo);
    string stopword;
    while(getline(archivo,stopword,',')) {
    diccionario[stopword] = 1;
    }
    archivo.close();
}

Stopwords::~Stopwords() {
    delete instance;
}


Stopwords* Stopwords::getInstance() {

    if (instance == NULL) {
       instance = new Stopwords ();
    }
    return instance;
}

map<string,short> Stopwords::getMap(){
    return diccionario;
}

It's not relevant but in the initialization, i read a bunch of words from a file and i save them in a map instance.

Thanks

Answer 1

The problem is that you never call the destructor or delete on the instance pointer by hand, i.e. from outside the class. So the delete inside the destructor will never get executed, meaning the destructor will never get executed, meaning the delete will never get executed, meaning... You see what you did there? Your destructor is indirectly calling itself which will not go well. And you never call it from the outside, so it never gets called at all - luckily.

You should change your singleton implementation, maybe a Meyers singleton (look it up), or even better not use a singleton at all. In cases like this one, where they are data sources, there are just too much weaknesses of the pattern to deal with.

Answer 2

Stopwords::~Stopwords() {
    delete instance;
}

This is the destructor for instances of the class. You probably intended this function to be called when the program ends, as though it were a kind of 'static' destructor, but that's not what this is.

So your destructor for instances of Stopwords initiates destruction of Stopwords instances; You've got an infinite loop here, which you never enter. If you do get into this loop then the program will probably just crash.

There's a simpler way to do singletons: Instead of keeping the instances as a static class member that you allocate manually, simply keep it as a static function variable. C++ will manage creating and destroying it for you.

class Stopwords {   
public:
    static Stopwords &getInstance() {
        static Stopwords instance;
        return instance;
    }

    ~Stopwords();
    std::map<std::string,short> getMap();

private:
    Stopwords();
    std::map<std::string,short> diccionario;
};

Also, you should mark member functions that don't need to modify the class as const:

std::map<std::string,short> getMap() const;

Answer 3

1. You allocate memory for instance with new. So the instance will be alive until delete is called.
2. Destructor is called in the case an instance of a class going to be killed.
3. Nothing kills (using delete) your instance, but destructor itself.

So the conclusion is that your instance is never killed.

Usually, when you use singleton you don't care to kill it before the program finishes. Why do you need this?

If you don't you better use static keyword to make explicit the fact that it is alive until the program finishes.

static Singleton& getInstance()
{
     static Singleton s;
     return s;
}