🔒 Locked

Question on singleton programming

Started by eduwushu Dec 11, 2008 at 2:57 AM 18 replies 2.7k views

Original Post

Dec 11, 2008 at 2:57 AM

Hi all, I've encountered some problems trying to implement a singleton pattern for a engine i'm developing. In that engine I needed a Renderer object. This renderer object must have subclasses in order to make it API independent. I will have a D3DRenderer and a OGLRenderer subclass for the Renderer class. I need in the engine only one object of the type of one of the two subclasses, but I need to access this object using the generic interface provided by the Renderer class. I thought in doing something like this: --FILE IRenderer.h-- class IRenderer { protected: static IRenderer* m_pRenderer; static void SetRenderer(IRenderer* renderer){m_pRenderer=renderer;} IRenderer(){} public: static IRenderer* GetRenderer(){return m_pRenderer;} virtual ~IRenderer(){} }; IRenderer* IRenderer::m_pRenderer=NULL; --FILE CD3DRenderer.h-- class CD3DRenderer : public IRenderer { private: LPDIRECT3DDEVICE9 m_pD3DDevice; IDirect3D9* m_pD3D9Interface; CD3DRenderer() { m_pD3D9Interface=NULL;m_pD3DDevice=NULL; } long InitD3D(D3DINITDATA* data); public: virtual ~CD3DRenderer(); static void Init(D3DINITDATA* data); }; --FILE CD3DRenderer.cpp-- void CD3DRenderer::Init( D3DINITDATA* data ) { if(m_pRenderer==NULL) { //Construct the new Renderer using the data specified m_pRenderer=new CD3DRenderer(); if(E_FAIL==((CD3DRenderer*)m_pRenderer)->InitD3D(data)) { delete m_pRenderer; m_pRenderer=NULL; } } } long CD3DRenderer::InitD3D( MIRAGED3DINITDATA* data ) { if(NULL==(m_pD3D9Interface=Direct3DCreate9(D3D_SDK_VERSION))) { return E_FAIL; } [...] D3DPRESENT_PARAMETERS d3dpp; [...] if( FAILED( m_pD3D9Interface->CreateDevice( D3DADAPTER_DEFAULT, D3DDEVTYPE_HAL,data->hWnd,vp,&d3dpp, &m_pD3DDevice ) ) ) return E_FAIL; return NOERROR; } CD3DRenderer::~CD3DRenderer() { m_pD3DDevice->Release(); } With this code the logical way to use the renderer object will be this: CD3DRenderer::Init(&somedata); //First initialize the singleton object IRenderer* renderer=IRenderer::GetRenderer(); (Do something with the renderer) The problem is that when I call to IRenderer::GetRenderer() from an application I get always the NULL pointer. I set a breakpoint inside the Init() method and the renderer object is being initialized OK (it is not setting the m_pRenderer varible to NULL). So I think there must be something that I have not took into account. Any ideas?? Thanks beforehand.

DevFred

Dec 11, 2008 at 4:07 AM

Quote:
Original post by eduwushu
I need in the engine only one object

That does NOT imply you should use the singleton pattern. The singleton pattern prohibits multiple instances of a class because it would be an ERROR if there existed more than one. Just instantiate one object in the engine and be done with it. No need for singletons.

eduwushu

Dec 11, 2008 at 7:13 AM

I know but this seems to be the perfect place to use a singleton pattern, doen's it? I must have in my application a single instance of an object which must be globally visible to other components. And i must ensure that no one can create another instance of it.

Anyway, I want to understand now what can be happening with this code and why it is not working, just as a programming curiosity. Can you see the error?

Evil Steve

Dec 11, 2008 at 7:23 AM

Are you sure that InitD3D() is succeeding? You can use the debugger to find out if the renderer pointer is actually set, or if it's being set and then reset for some reason.

DevFred

Dec 11, 2008 at 7:34 AM

Quote:
Original post by eduwushu
I must have in my application a single instance of an object

Why don't you simply create as many instances as you need? In this case, one? Why do you have to prevent other instances?

Quote:
Original post by eduwushu
which must be globally visible to other components

Nothing MUST be globally visible if you have a good OO design. Why should the Renderer be visible to the sound system, the input system or the game logic?

Quote:
Original post by eduwushu
And i must ensure that no one can create another instance of it.

Why?

Antheus

Dec 11, 2008 at 7:37 AM

Quote:
Original post by eduwushu
I know but this seems to be the perfect place to use a singleton pattern, doen's it? I must have in my application a single instance of an object which must be globally visible to other components.

Singletons don't have initialization function. They aren't created or destroyed. They always exist.

They are also inherently unsuitable for polymorphic behavior.

Quote:
And i must ensure that no one can create another instance of it.

Looky here:

CD3DRenderer::Init(&somedata);CD3DRenderer * p1 = CD3DRenderer::getInstance();CD3DRenderer::Init(&someother);CD3DRenderer * p2 = CD3DRenderer::getInstance();CD3DRenderer::Init(&somemoredata);CD3DRenderer * p3 = CD3DRenderer::getInstance();assert(CD3DRenderer::getInstance() == &data);assert(p3 == p1);assert(p2 == &somemoredata)

While this seems like it cannot happen, it is exactly why you run into problems - singletons cannot have Init functions.

You assume your singleton can only have value set once, but in your non-singleton singleton that isn't true. There is a reason why singletons cannot have init functions or constructors visible from outside. See below.

Quote:
Anyway, I want to understand now what can be happening with this code and why it is not working, just as a programming curiosity. Can you see the error?

Quote:

void CD3DRenderer::Init( D3DINITDATA* data ){if(m_pRenderer==NULL)

Due to static initialization order fiasco, m_pRenderer might not have been set to NULL by the time you call Init. And since variables in C++ aren't auto-initialized, it may be 0, or it may have garbage value.

The canonical singleton implementation in C++ is:

struct Singleton {  static Singleton & getInstance() {    static Singleton instance;    return instance;  }};

ToohrVyk

Dec 11, 2008 at 7:39 AM

Quote:
Original post by Antheus
They are also inherently unsuitable for polymorphic behavior.

class MySingletonFrobnicator : IFrobnicator ?

Quote:
The canonical singleton implementation in C++ is:
struct Singleton {  static Singleton & getInstance() {    static Singleton instance;    return instance;  }};

So long as you don't have multiple threads [smile]

eduwushu

Dec 11, 2008 at 8:00 AM

Thanks for all the replies.
I will take them into consideration to rewrite my code.

According to them, you would then recommend me to have a global variable of IRenderer type, or maybe an attribute of IRenderer type in some CEngine class?

When i was debuggin the application I noticed this issue:

--TestApp.cpp--

[Breakpoint]
CD3DRenderer::Init(&data);

When I hit the breakpoint placed before the call to Init I add a watch variable in the debugger with the value of the &IRenderer::m_pRenderer.

Then I enter the Init method and add another watch with the value &m_pRenderer. And this last direction was different from the direction I get outside the Init method. This sounded very strange to me.

_moagstar_

Dec 11, 2008 at 8:11 AM

Quote:
Original post by Antheus

Singletons don't have initialization function. They aren't created or destroyed. They always exist.

The singleton implementation you posted does have initialisation and shutdown functions, namely the constructor and destructor of Singleton (albeit compiler generated). It will be constructed the first time you call the function, and destroyed at some point during shutdown...you can't assume that they always exist. Construction isn't that much of a problem (assuming only 1 thread) but during shutdown you could have problems making that assumption if you have two singletons that interact because you don't know the order in which they are destroyed.

venzon

Dec 11, 2008 at 9:22 AM

Quote:
Original post by eduwushu
Thanks for all the replies.
I will take them into consideration to rewrite my code.

According to them, you would then recommend me to have a global variable of IRenderer type, or maybe an attribute of IRenderer type in some CEngine class?

If it were me, I would do the latter: contain the renderer in an higher level class such as an engine class, like you've mentioned. The engine class knows about the renderer and about other high-level classes (like, say, your top-level scenegraph class), and passes information between them as necessary. There was just a huge discussion on singletons and globals a couple of weeks ago.

jpetrie

Dec 11, 2008 at 10:37 AM

Quote:

Thanks for all the replies.
I will take them into consideration to rewrite my code.

According to them, you would then recommend me to have a global variable of IRenderer type, or maybe an attribute of IRenderer type in some CEngine class?

Removing singletons is a trivial problem: delete the singleton class machinery, and replace the singleton with a single global instance somewhere, then modify all code that used to talk through the singleton machinery (GetInstance(), or whatever) to talk directly to the global. This should be your first step.

Removing globals is harder, and it gets harder the longer you let them sit around and cruft up your code. In the process of fixing up that first step so that things compiled, you should have touched every file that referenced the singleton-turned-global. So you should have an idea of how pervasive the object is.

For every subsystem that relies on the global, you want to decide if you really need that subsystem to use that global, or if the subsystem can be rewritten to avoid. If, for example, you require only a small set of properties off that global -- perhaps a render-area width and height? -- then you can remove references to that global from within the subsystem and instead have that subsystem require the width and height on construction. If the global really seems to be tightly integrated with the subsystem, then you have to make a cost/benefits decision: if its too costly to refactor the global out entirely right now, then instead refactor the access out. That is, have the subsystem accept a pointer or reference to the object on initialization, and have the subsystem talk to that stored pointer rather than the global. If possible you can return to the afflicted subsystem later to clean it up more.

Once you have done this with all subsystems that depend on the global, you will no longer need the global and you will be on the path to what is likely a cleaner design.

(Yes, it is possible to get into a situation where you are passing many objects around as parameters to constructors and initialization functions this way, and that's a good thing -- it makes it quite obvious that the subsystems in question are not loosely coupled and have potentially serious dependency issues; it encourages you to think about how you can reduce those dependencies so you can pass less. Remember, if all those instances you're passing were global, you wouldn't actually have a better design, you'd still have a very promiscuous subsystem. It'd just be more secretive about it).

Antheus

Dec 11, 2008 at 2:02 PM

Quote:
Original post by _moagstar_

The singleton implementation you posted does have initialisation and shutdown functions, namely the constructor and destructor of Singleton (albeit compiler generated).

Yes, I don't like to waste time stating the obvious. Singleton has private constuctor.

Quote:
It will be constructed the first time you call the function, and destroyed at some point during shutdown...you can't assume that they always exist.

If a tree falls down in the forest and nobody is there to hear it, does it make a sound?

The only and only way to get a singleton is to call getInstance(). Whether it's physically manifested before that time is irrelevant. Call to getInstance(), the one and only way to get this singleton will always return exactly the same value whenever called. As such, it "always exists".

But in case of manual initialization, calling getSingleton() may or may not return a valid instance.

Quote:
Construction isn't that much of a problem (assuming only 1 thread) but during shutdown you could have problems making that assumption if you have two singletons that interact because you don't know the order in which they are destroyed.

Singletons are not destroyed, and don't have shutdown order. Just like frogs don't have wheels.

If shutdown is a problem, then singletons are wrong design choice. Singletons do not shutdown and are not destroyed - from design perspective. There is no concept in singletons that would allow for specific order of destruction.

Why do people refuse to let go of a broken concept and instead prefer to waste time on unsolvable non-deterministic problems is beyond me.

The problems you mention exist only because singletons are wrong choice in that case, so workarounds must be provided.

_moagstar_

Dec 12, 2008 at 2:19 AM

Quote:
Original post by Antheus
Yes, I don't like to waste time stating the obvious. Singleton has private constuctor.

I don't think it's that obvious, it's a subtle problem that can lead to errors.

Quote:

If a tree falls down in the forest and nobody is there to hear it, does it make a sound?

The only and only way to get a singleton is to call getInstance(). Whether it's physically manifested before that time is irrelevant. Call to getInstance(), the one and only way to get this singleton will always return exactly the same value whenever called. As such, it "always exists".

Hence why I said 'construction is not a problem', provided you are working with only one thread.

Quote:

Singletons are not destroyed, and don't have shutdown order. Just like frogs don't have wheels.

I'm pretty sure that is wrong, are you saying that if Singleton has a destructor it will never be called? If it is called can you guarentee that any other singletons that you are assuming always exist haven't already been destroyed?

Quote:

Why do people refuse to let go of a broken concept and instead prefer to waste time on unsolvable non-deterministic problems is beyond me.

The problems you mention exist only because singletons are wrong choice in that case, so workarounds must be provided.

Agreed, I wasn't trying to hold onto a broken concept, I'm not advocating the use of singletons in any way. I was just trying to point out that 'Singletons always exist' is an incorrect and dangerous assumption.

ToohrVyk

Dec 12, 2008 at 2:31 AM

Quote:
Original post by _moagstar_
I'm pretty sure that is wrong, are you saying that if Singleton has a destructor it will never be called? If it is called can you guarentee that any other singletons that you are assuming always exist haven't already been destroyed?

A singleton is neither created nor destroyed: it just exists, so that whenever you try to access its instance, it will be there. In the same vein, an std::vector doesn't contain a variable that stores its size, so there's no way you can read (or change) that variable.

Of course, behind the scenes, a singleton has to be created and destroyed, because that's how the language works, and an std::vector has to store a variable that stores its size, because that information has to exist somewhere. But this happens behind the scenes: if your user code relies on the fact that a singleton is created or destroyed, then your user code is just as wrong as if it relied on an std::vector having a size member.

_moagstar_

Dec 12, 2008 at 6:04 AM

Quote:
Original post by ToohrVyk
Quote:
Original post by _moagstar_
I'm pretty sure that is wrong, are you saying that if Singleton has a destructor it will never be called? If it is called can you guarentee that any other singletons that you are assuming always exist haven't already been destroyed?
A singleton is neither created nor destroyed: it just exists, so that whenever you try to access its instance, it will be there. In the same vein, an std::vector doesn't contain a variable that stores its size, so there's no way you can read (or change) that variable.

Of course, behind the scenes, a singleton has to be created and destroyed, because that's how the language works, and an std::vector has to store a variable that stores its size, because that information has to exist somewhere. But this happens behind the scenes: if your user code relies on the fact that a singleton is created or destroyed, then your user code is just as wrong as if it relied on an std::vector having a size member.

I'm not sure I understand the analogy with std::vector, are you saying that ensuring instance() actually provides a valid instance is an implementation detail of the singleton class?

If client code that relies on the fact that a singleton is created or destroyed is wrong, then any line of code that uses the canonical singleton is wrong, because in practise client code calling some_singleton::instance().some_mem_fn() does rely on the fact that the singleton has been created and not destroyed, and that is precisly what the 'canonical' implementation cannot guarentee.

And that's my point - client code can happily write that line of code under the assumption that the singleton always exists - but with the singleton implementation that Antheus posted it's an assumption based on what you expect a singleton to provide, not what the implementation actually gives you.

Antheus

Dec 12, 2008 at 8:32 AM

Quote:
Original post by _moagstar_

If client code that relies on the fact that a singleton is created or destroyed is wrong, then any line of code that uses the canonical singleton is wrong, because in practise client code calling some_singleton::instance().some_mem_fn() does rely on the fact that the singleton has been created and not destroyed, and that is precisly what the 'canonical' implementation cannot guarentee.

The only reason for existence of Singleton pattern (instead of just using a global) is that it absolutely positively guarantees that it always exists from user's perspective.

If you drop the "always exists" from singleton implementation, it becomes just regular global.

See the Singleton Pattern. While there should be no way to call getInstance() and get invalid value back, regardless of where it's called, the C++ implementation shown there states:

Quote:
In the above example, the first call to Singleton::GetInstance will initialize the singleton instance. This example is for illustrative purposes only; for anything but a trivial example program, this code contains errors.

And then doesn't bother explaining why.

_moagstar_

Dec 12, 2008 at 10:27 AM

Quote:

In the above example, the first call to Singleton::GetInstance will initialize the singleton instance. This example is for illustrative purposes only; for anything but a trivial example program, this code contains errors.

[grin] That's the best caveat ever.

Guthur

Dec 12, 2008 at 10:53 AM

This thread got me thinking, and made me realise my singleton classes are usually bad.

So decided to make a generic templated singleton holder. It is just a tad over engineer though, maybe :p Nice wee distraction for a while

Any so here comes the code, I have actually put more effort into completeness here than usual, ie proof and more than one scenario... 2 but at least its a start :)

So here it is:

Singleton class code:

////ALLOCATORS////Defaulttemplate  < typename T >class Standard {public:	static T* Allocate() { return new T(); }};//Pass an Inttemplate  < typename T >class PassInt {public:	static T* Allocate(int arg) { return new T(arg); }};////INSTANCE HOLDER ensures the object is //really a singleton even when differing //allocators are used//template<typename T>class InstanceHolder{public:	static T* pInstance;};template<typename T>T* InstanceHolder<T>::pInstance = 0;////Our Generic singleton wrapper (default allocation)//template <typename T, class Alloc = Standard<T> >class GenericSingleton{public:	GenericSingleton();	T* GetSingleton() {return Instance.pInstance;}private:	InstanceHolder<T> Instance;};template <typename T, class Alloc>GenericSingleton<T, Alloc>::GenericSingleton(){	if(!Instance.pInstance)		Instance.pInstance = Alloc::Allocate();}////Our Generic singleton wrapper //(specialisation for Passing an int allocation)//template <typename T>class GenericSingleton< T, PassInt<T> >{public:	GenericSingleton(int arg);	T* GetSingleton() {return Instance.pInstance;}private:	InstanceHolder<T> Instance;};template <typename T>GenericSingleton<T, PassInt<T> >::GenericSingleton(int arg){	if(!Instance.pInstance)		Instance.pInstance = PassInt<T>::Allocate(arg);}//Our singleton or not as the case is; the template gives us singleton behaviour not the classclass SomeClass{public:	SomeClass() { SingleInt = 2; }	SomeClass(int arg) { SingleInt = arg; }	int DoubleIt() { return SingleInt * 2; }private:	int SingleInt;};

Unit Test Functions:

//Unit Test Functionsint Foo(){	GenericSingleton<SomeClass> GS;	return GS.GetSingleton()->DoubleIt();}int Goo(){	GenericSingleton<SomeClass, PassInt<SomeClass> > GS( 5 );	return GS.GetSingleton()->DoubleIt();}

First test both values returned equal 4:

int _tmain(int argc, _TCHAR* argv[]){	int val2 = Foo();	int val1 = Goo();	return 0;}

Second test both values returned equal 10

int _tmain(int argc, _TCHAR* argv[]){	int val1 = Goo();        int val2 = Foo();	return 0;}

This is only foundation of a possible way to implement safe single shot singletons, but like I mentioned it probably is over engineered :)

_moagstar_

Dec 12, 2008 at 11:03 AM

Quote:
Original post by Guthur
This thread got me thinking, and made me realise my singleton classes are usually bad.

There are very few good implementations, and theres more than just handling of dead references to consider if you want to get it right.

Check out this for probably one of the most complete implementations of a singleton wrapper.

[Edited by - _moagstar_ on December 13, 2008 3:03:14 AM]

Guthur

Dec 12, 2008 at 11:39 AM

Thanks for the Loki lib link _moagstar_, should be getting Modern C++ Design soon, which will be interesting.

[Edited by - Guthur on December 13, 2008 5:39:57 AM]

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.