views:

647

answers:

4

Due to the flooding examples of implementing logger using Singleton pattern, I just written a simple C++ logger in the same approach for my program. However, since the famous double-checked locking approach is known to be no more thread-safe, I wonder if I should:

1) Forget about the use of Singleton pattern in this case?

2) Continue to use double-checked locking even though it is unsafe?

3) Use the expensive pure sync lock method for every access to its public interfaces?

4) ... any suggestion?

+11  A: 

Use Meyers Singleton. If you are using using gcc at least initialization is thread-safe.

class Singleton{
   Singleton(){
    //This is threadsafe in gcc, no mutex required
   }
   static Singleton * instance(){
      static Singleton myinstance;
      return &myinstance;
   }
};

gcc guards static locals construction unless you disable with -fno-threadsafe-statics, I recently wrote about that here

Arkaitz Jimenez
Wouldn't you want instance() to be static?
Aaron
Aaron, you're right.
Arkaitz Jimenez
+1 for this note, I once spent enough time around this problem with WindRiver C/C++ compiler (former Diab).
Roman Nikitchenko
It should be `return `.
mash
In my case, there are two initializations need to be done: init the instance (internally), and init various log settings (by client code, through provided interface).
shiouming
@shiouming, if you do it inside the Singleton constructor you're protected there.
Arkaitz Jimenez
Instead of returning `Singleton *`, return `Singleton ...lines of code...delete instance;`
mos
Please dont return a pointer. If you do that you open up a whole can of worms regarding who owns the pointer (and thus who should release it). By returning a reference you are indicating your code retains ownership and thus will handle the destruction. (Note in modern C++ code you should hardly ever be returning a RAW pointer).
Martin York
+1  A: 

In applications with threads, I prefer to use singletons with an initialize() function and asserts to make sure that the initialize() is used before the first instance(). Call initialize() from the main thread. I don't think that lazy instantiation is really the key feature of a singleton, especially for a logger.

While Arkaitz's answer is more elegant, my answers avoids threading issues on all platforms with the cost of 1 extra function and some instantiating concerns during startup for singletons with dependencees (helped by asserts and ofcourse: use singletons judiciously).

stefaanv
I tend to like this answer, but with the caveat that you then can't use the logging during pre-initialization code. This isn't usually a big deal, but I've had it bite me once or twice.
Michael Kohne
@Michael: when we decided for this approach, I actually suggested using your answer: accessing in the main thread. But this approach works for us and it makes sure that the singleton is always initialized in the main thread. If the access would be removed from the main thread, we would only find the first time there is a problem (in the field?)
stefaanv
My existing code is pretty close to this approach. However, I also wondering if I should assume that logger's client code will always use/initialize the logger correctly, or should I perform any checking internally.
shiouming
@shiouming: use asserts: in debug-mode you can't do anything else than first initialize(), especially since you have some user-settings. Do you have a dependency issue with first reading the user settings (without logger) and only then initializing the logger? Can this be solved with first using default settings?
stefaanv
Yup, existing implementation uses default setting upon first call to getInstance(), either if client has not initialize the logger's setting, or it is not initialized correctly.
shiouming
+1  A: 

One approach would be to make sure that your first access to the logger comes before your app starts a second thread. By accessing the singleton at a time when you KNOW that there isn't any contention, you make sure that subsequent accesses will always find a pre-existing object and you should completely avoid the problem.

Michael Kohne
A: 

You do not really need separate Initialize() function as this will just contaminate your singleton interface. Just get singleton instance

VERIFY(NULL != Logger::Instance());

before any other thread has chance to access it.

BostonLogan
You should not need to check a singelton for NULL. It exists or it does not exits. Returning a reference to singelton gurantess it exists.
Martin York
Thanks Mark. I agree that singleton getter must return reference, not pointer. My line refers to the snipped about Meyers singleton above that does return it by pointer. Although it appears that it can not fail, you never know who will be modifying your code down the road. So VERIFY here is just a time capsule for future programmer.
BostonLogan