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

Solution 1

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;

Solution 2

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.

Solution 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;
}

Comments

[Arne Mertz]

This wont help anything if the destructor is not actually called from the outside.

[odedsh]

He could also use std::unique_ptr<Stopwords> instead of a raw pointer.

[Admin]

Also, a better way of doing this would be just referencing instance instead of having it as a pointer, e.g. return &instance in getInstance()

[Arne Mertz]

@odedsh that would be one possible solution, as the unique_ptr destructor will get called from the outside, implicitly.

[Ehsan Ab]

Whenever getInstance is called a new instance is created and returned? Wouldn't this defeat the purpose of singletons?

[bames53]

@EhsanAb No, every time getInstance is called it returns the same object.

[Ehsan Ab]

ahh I see/ it's static. Didn't see it

[Zuzu Corneliu]

An important word of warning: the Stopwords instance is created on demand! That means its constructor will only be called (once) the first time getInstance() is called. You have to keep this lazy-instantiation behavior in mind for cases you'll be calling getInstance() from concurrent contexts (multithreading). To avoid this problem move the "static Stopwords instance;" outside of getInstance() definition and into the global context (*.cpp) - that way it will be created before the main procedure (not lazily).

[Zuzu Corneliu]

Extra note regarding proposed solution: it's even better to make the Stopwords instance a static member of the Stopwords class, otherwise Stopwords constructor cannot be private.

[bames53]

@CorneliuZuzu "You have to keep this lazy-instantiation behavior in mind for cases you'll be calling getInstance() from concurrent contexts (multithreading)." Since C++11 this lazy initialization has been multi-thread safe.

[Zuzu Corneliu]

@bames53 Oh, good to know. But that also means that behind the scenes a lock must be taken+released every time getInstance() is called. A runtime overhead which I'd say is not worth it.

[bames53]

@CorneliuZuzu There are more efficient ways to implement it than a full-blown lock that has to be taken every time so I would not expect that to be an issue.

[Zuzu Corneliu]

@bames53 I'd be curious how. If thread A and thread B are both executing getInstance() for the first time AND at the "same" time it's clear that one would have to wait for the other. From that moment on (subsequent calls), I guess an atomically updated function pointer could be employed such that the lock-logic is not entered again. That sounds pretty good but I wonder if in practice something similar really is happening.

[Josh Greifer]

A note on whether to use static T *instance vs static T instance. If T itself is large, the former may be preferable, as it does heap allocation rather than static data allocation. This makes the executable image size smaller, reserving only sizeof(T *) bytes in the data segment, rather than sizeof(T) bytes.

[Rodrigo]

This approach also allows to do Stopwords stopwords = Stopwords::getInstance() or Stopwords &stopwords = Stopwords::getInstance(). Be careful in the first case because you are calling the copy-constructor and the object in stopwords will no be the same as the object stored in instance

[bames53]

@Rodrigo To prevent that you can delete the copy constructor.