CODEWITHSUNDEEP
c++c++11singleton
Mark Guo
Mark Guo

Reputation: 213

Is it right way to create singleton class by weak_ptr

I create a parent class to handle singleton pattern with smart pointer:

.h file:

template<class singleType>
class Singleton
{
public:
    static std::shared_ptr<singleType> GetInstance();

private:
    static std::weak_ptr<singleType> m_singleObject;
};

.cpp file:

template<class singleType>
std::shared_ptr<singleType> Singleton<singleType>::GetInstance()
{
    auto shareObject = m_singleObject.Lock();
    if (!shareObject)
    {
        shareObject.reset(new singleType);
        m_singleObject = shareObject;
    }

    return shareObject;
}

Not sure it is the right way to use smart pointer? Any idea?

Many Thanks

Upvotes: 14

Views: 3257

Answers (4)

Gerrit
Gerrit

Reputation: 435

The pros and cons of this implementation are already discussed. But there are a bunch of bugs:

1) As this is a template you have to move your implementation into the header or the linker cannot find it.

2) The .lock() function of the weak_ptr is not in capitals.

3) Don't forget to instantiate 

template<class singleType> 
std::weak_ptr<singleType> Singleton<singleType>::m_singleObject;

4) Better use shareObject = std::make_shared<singleType>(singleType()); instead of the new: http://herbsutter.com/gotw/_103/

5) As Konrad mentioned: it's not thread-safe.

Upvotes: 4

Konrad Rudolph
Konrad Rudolph

Reputation: 545995

Your code isn’t thread safe.

The name lock may suggest that concurrent access is blocked but in fact there’s no such guarantee: when multiple threads concurrently call your GetInstance function you will get several instances instead of guaranteeing a single one.

You need to create an explicit lock for the whole duration of the GetInstance function’s lifetime. Note that this is of course not very efficient.

Upvotes: 4

Danny
Danny

Reputation: 2291

I did some research so now I'll post an answer.

The code all looks like it should work and is a correct usage of smart pointers. The only question is how exactly you want the singleton to behave. This should behave like a textbook singleton EXCEPT for the behavior that if nothing currently has a pointer to the singleton, it will delete itself. This behavior is something that really depends on the implementation of your program. If you want the singelton to only exist when it is in use, then I'd say go for it.

I would just avoid creating and destroying the singelton too often especially if the construction and deconstruction are particularly intensive. If it is constantly getting created and deleted then you are probably better off with a more standard singleton implementation. The standard singleton behavior tends to be that a singleton is only created once during the duration of the program running and is never deleted.

I think this is a clever implementation given you have a use for it and I might have to borrow the idea.

Upvotes: 4

RandyGaul
RandyGaul

Reputation: 1915

To the best of my knowledge this should be fine. You'll avoid out of order destruction issues, but it could be a problem to create new instances after your initial instance is created. This singleton will have only one instance alive at any time, but over the course of the program running more than one instance may have been alive overall.

This destruction and recreation may also be unwanted in terms of performance, not only in side-effects.

Upvotes: 1

Related Questions