Four interesting questions - Part III

This is the third post in this series. This question deals with C/C++ concurrency. The code looked somewhat similar to this and the question was whether this Singleton class is thread safe? Explain why not and how to fix or if yes why?

static Singleton * Singleton::instance();

Singleton * Singleton::instance() {
if (pInstance == 0) {
Lock lock;
pInstance = new Singleton;
}
return pInstance;
}

Put it in a another way when the static instance() member function is called in two threads concurrently will the Singleton property of the class hold. (ie. only a single instance will be created). The problem in here is when the instance() member function is called in two threads concurrently, it can be stopped in arbitrary locations (Actually this gets converted into machine code and it get stopped).

If you look closely you will find out that the following can occur. If two threads are there, first one tests to see whether pInstance equals NULL, afterwards which is immediately moved into runnable state (gives up CPU), then the second thread is scheduled and finds out that pInstance is NULL goes on to initiates it. When the first thread is resumed it will also create a new Singleton object (it resumes where it was stopped) and violates the singleton property. So far so good.
The code above is wrong. But why would someone write awkward code as above in first place with a lock inside a condition. Its not obvious what he/she was trying to do unless you know the double checked locking design pattern. The idea hear is if you write something like,

static Singleton * Singleton::instance();

Singleton * Singleton::instance() {
Lock lock;
if (pInstance == 0) {
pInstance = new Singleton;
}
return pInstance;
}

every time you access the singleton object (if declared private in the class) the member function should acquire the lock. (Writing it without locks provides no thread safety). So the intention of the first code segment is to reduce locking (Need to lock only when creating). I think the desired answer for this question was:

static Singleton * Singleton::instance();

Singleton * Singleton::instance() {
if (pInstance == 0) {
Lock lock;
if (pInstance == 0) {
pInstance = new Singleton;
}
}
return pInstance;
}

Where after locking it checks whether the object is still NULL, if so just return the original object.

Interesting, later I came to know that there is no correct way to write a answer to this problem in standard C++. Above solution may break in a standard complaint C/C++ implementation. It seems this is because C/C++ does not define a concurrency memory model or threading in standards but in most platforms the above code will not fail provided pInstance is declared volatile. One of the excellent articles describing a way this will break is by Scott Meyers and Andrei Alexandrescu two leading authorities on C++.