Sign in to follow this  

Singleton Design

This topic is 2390 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello,

I have some singletons in my program. For example an App singleton (controls game loop etc) and a Logger and MemoryManager singleton. I guess it is preferable to control the lifetime of the singletons, for example I want to be sure logging in the App destructor is possible. Therefore I thought about this design: The App singleton is kinda like a singleton manager. App is created first, creates all other singletons and is also responsible for destroying them.

The code would roughly look like this:
[code]
// Main Singleton
class App {
private:
App();
App(const App& rhs);
~App();
App& operator=(const App& rhs);

static App* instance;

public:
static App* createInstance() {
if(instance == NULL)
instance = new App();
return instance;
}

static App* getInstance() {
assert(instance);
return instance;
}

static void destroyInstance() {
if(instance)
delete instance;
}

private:
App() {
// Andere Singletons erzeugen:
Logger::createInstance("MyLogger");
MemoryManager::createInstance();
...
}

~App() {
// Andere Singletons zerstören:
Logger::destroyInstance();
MemoryManager::destroyInstance();
...
};

//Usage:
int main() {
App* a = App::createInstance();
// Now App and all other singletons are usable
a->foo();
...

App::destroyInstance();
}
[/code]
Do you think this will work? Or do you see any big problems/design flaws?
Do you think the design is good?

PS: I am NOT using multithreading.

Share this post


Link to post
Share on other sites
Looks fine (its better than the static singleton approach). I do prefer to have somehtign actually own it and not have it created on first use and destroyed in some random order at exit. I would just use the constructor/destructor instead of create instance/destroy instance. Though perhaps your idea is more explicit in describing whats going on. I would definatly make it fail outright if you try to create an instance when one already exists (rather than just returning the current instance), otherwise, whoever tried creating it will assume they now own it and are responsible for destroying it too.

[code]static App *instance = NULL;

class App
{
public:
App()
{
assert(instance == NULL); // Throw some error, force it to crash so the programmer only ever calls it once by design.
instance = this;
}
~App()
{
assert(instance == this);
instance = NULL;
}
static App* GetInstance()
{
// Throw an error if no instance exists
return instance;
}
};[/code]

I would first think about why you need to make this App class globally accessable though.

Share this post


Link to post
Share on other sites
Note that your code becomes a lot simpler when not using singletons here:[code]class App {
public:
App() : m_Logger("MyLogger"), m_Memory() { m_Logger.Log("Starting up"); }
~App() : { m_Logger.Log("Shutting down"); }
private:
Logger m_Logger;
MemoryManager m_Memory;
};

//Usage:
int main() {
App a;
// Now App and all app's members are usable
a.foo();
...
}[/code]No concerns about order of creation/destruction either...

What does all the added complexity of using the singleton pattern give you here ([i]besides the disadvantage of turning things into [url="http://c2.com/cgi/wiki?GlobalVariablesAreBad"]global variables[/url][/i])?

Share this post


Link to post
Share on other sites
Thanks for your responses.

@Nanoha: Just out of curiosity: Why do you use a module static global variable: static App *instance = NULL; ?
Is that intentional or did you mean a class variable: App* App::instance = NULL;

@Hodgman: You are right and actually I do not like singletons. But in this case it makes sense because more than one App instance makes no sense (app controls the window message loop and there only can be one message loop etc.).
Plus the logger is needed everywhere and it would be very tedious to pass through a logger pointer to all the classes that might need logging.

Share this post


Link to post
Share on other sites
[quote name='schupf' timestamp='1306664923' post='4817039']
Thanks for your responses.

@Nanoha: Just out of curiosity: Why do you use a module static global variable: static App *instance = NULL; ?
Is that intentional or did you mean a class variable: App* App::instance = NULL;

[/quote]

I did mean static, that bit would usually go in the implementation(cpp) and not the header. It could go in the class definition.

If you want the logger to be globally accessable, then make just the logger globally accessable (and not the App). If you want to enforce the App as a singleton, then just have a flag saying if an instance has been created and force it to crash/throw an exception.

[code]class App
{
private:
static bool instanceExists = false;
public:
App()
{
// Check instance doesn't exist
if(instanceExists) { throw somehting; }
instanceExists = true;
}
~App()
{
instanceExists = false;
}
};[/code]

Share this post


Link to post
Share on other sites
[quote name='schupf' timestamp='1306664923' post='4817039']
But in this case it makes sense because more than one App instance makes no sense (app controls the window message loop and there only can be one message loop etc.).[/quote]
An alternative would be to create a single App object, and then pass it forward to any other objects that need access to it.

[quote]Plus the logger is needed everywhere and it would be very tedious to pass through a logger pointer to all the classes that might need logging.[/quote]
There are alternatives to singletons here as well. For example, you could make the logger a simple global object.

Share this post


Link to post
Share on other sites
C++ singletons seem really silly to me. Why not just use a namespace?:
[code]App::Log(const std::string& s)[/code]
Or create a single instance of the class, using what you would normally think of the class name:
[code]class App_
{
void Log(const std::string& s);
};
App_* App = new App_;[/code]
Why all these drastic measures to try to conform to a paradigm C++ doesn't support?

Share this post


Link to post
Share on other sites
[quote name='Josh Klint' timestamp='1306688639' post='4817159']
Why all these drastic measures to try to conform to a paradigm C++ doesn't support?
[/quote]

The most important reason whenever I use it: not having it created at some random point before main.

A somewhat constructed example.

[code]
class AutoRegister
{
AutoRegister() { globalMap[id] = this; }
};
[/code]

AutoRegister globalObject;


This can crash horribly, if globalMap is in some other .cpp, since you have no control if globalObject or globalMap is created first. In one case you're lucky, in the other you will waste a lot of time wondering why accessing the map will fail badly.

[code]
map<x,x>& registryMap()
{
static map<x,x> instance;
return instance;
}
[/code]

Now it can't crash, because calling "registryMap()[id] = this" will make sure that the map is instanced before you actually access it for the first time.

Personally I think it partially defeats the purpose of a singleton, if it has to be explicitely created and destroyed. Sure, you still make sure there can be one and only one (kind of), but all the automatism and safety is ripped right out of it.

Obviously, as long as you never ever access a global object from another global objects constructor, this shouldn't become an issue. A common example however would be static registration with a factory.

Making the logger a global object could suffer the same fate, if object constructors try to use the logger which tends to happen in the kind of classes that are tempting to make global as well (class MyEngine, etc.)

In this simple case you might notice that I don't bother wrapping the map in a dummy class, just to make some static "getInstance()" function.

Share this post


Link to post
Share on other sites
[quote name='Trienco' timestamp='1306731079' post='4817380'][code]class AutoRegister
{
AutoRegister() { globalMap[id] = this; }
};
AutoRegister globalObject;[/code][/quote]Going off topic, but when you least expect it, the linker is going to dead-strip that code and it's not going to make it into the final EXE, leaving you wondering why auto-registration has suddenly stopped working for certain classes...

Running code before main is [url="http://www.parashift.com/c++-faq-lite/big-picture.html#faq-6.15"]evil[/url].

Share this post


Link to post
Share on other sites
Its me again:)

I changed my design a little bit. App is not a singleton any longer. It is a normal class which just can be instantiated one time (but no global static access functions are available). Lets assume all my Singletons (Logger, MemoryManager) are Meyer singletons with a static local variable, e.g.:
[code]
static Logger& getInstance() {
static Logger logger;
return logger;
}

App::App() {
Logger::getInstance().log("App created.");
}

// App destructor:
App::~App() {
Logger.getInstance().log("App destroyed.");
}

int main() {
App* app = new App(..);
app->doStuff();
delete app;
}
[/code]
As you can see the Logger is used the first time in the App Ctor. Now I wonder WHEN the logger (and all other singletons) get destroyed.
Is it safe to use the logger in the App destructor and the destructor of all other singletons?

Share this post


Link to post
Share on other sites
The logger will be destroyed sometime during returning from main. It is safe to use it in the destructor of App, because that is called inside main(). It is not safe to call it in the destructor of other Singletons, unless they are deterministically destroyed before main() ends.

Running code outside main() is dubious.

One simple alternative is to make all these objects local to main. Then you can point global pointers at them when they are valid. Before they are destroyed, null out the pointers. To make it safe ensure that all your "global" classes do not use the global pointers to other sub-systems. Instead, they must be passed pointers/references. This way your constructor order will naturally force objects to be constructed in a particular order, and will alert you to circular dependencies (your Logger shouldn't be using the MemoryManager if the MemoryManager wants to use the Logger).

[code]
Logger *globalLogger = 0;
MemoryManager *globalMemoryManager = 0;

int main()
{
Logger logger("foo.log");
MemoryManager memoryManager(logger);

// Globals accessed before here will be NULL!

globalLogger = &logger;
globalMemoryManager = &memoryManager;

// Globals are safe!

Application application;
application.run();

// Goodbye globals...

globalLogger = 0;
globalMemoryManager = 0;
}
[/code]
You may want to export your globals through functions rather than directly as global pointers to prevent accidental assignment:
[code]
namespace
{
Logger *globalLogger = 0;
MemoryManager *globalMemoryManager = 0;
}

Logger &logger()
{
assert(globalLogger);
return *globalLogger;
}

MemoryManager &memoryManager()
{
assert(globalMemoryManager);
return *memoryManager;
}

int main()
{
// ...
}
[/code]

Share this post


Link to post
Share on other sites
[quote name='Hodgman' timestamp='1306733968' post='4817393']Going off topic, but when you least expect it, the linker is going to dead-strip that code and it's not going to make it into the final EXE, leaving you wondering why auto-registration has suddenly stopped working for certain classes...[/quote]

The ugliest part is where compilers are allowed to delay the initialization of static variables until they are actually read for the first time, which makes static factory registration such a ridiculously messy thing.

[code]
static bool isRegistered = register<X>();
[/code]

Looks simple and obvious, yet the compiler ignores the importance of all side effects in register() and decides "I won't do that until somebody reads isRegistered, even if that is never". Even a working version using registration objects stopped working when putting that code into a library. It was in the lib, but got stripped when building an application using that lib (actually that part worked, until you built an app using a dll using the lib).

Share this post


Link to post
Share on other sites
[quote name='Trienco' timestamp='1306773167' post='4817562']
[quote name='Hodgman' timestamp='1306733968' post='4817393']Going off topic, but when you least expect it, the linker is going to dead-strip that code and it's not going to make it into the final EXE, leaving you wondering why auto-registration has suddenly stopped working for certain classes...[/quote]

The ugliest part is where compilers are allowed to delay the initialization of static variables until they are actually read for the first time, which makes static factory registration such a ridiculously messy thing.

[code]
static bool isRegistered = register<X>();
[/code]

Looks simple and obvious, yet the compiler ignores the importance of all side effects in register() and decides "I won't do that until somebody reads isRegistered, even if that is never". Even a working version using registration objects stopped working when putting that code into a library. It was in the lib, but got stripped when building an application using that lib (actually that part worked, until you built an app using a dll using the lib).
[/quote]
Will that happen with statics created within a function as well?

Share this post


Link to post
Share on other sites

This topic is 2390 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this