I have ran into a situation that is very werid. Could anyone help me

This a C++ application built and debugged on Visual Studio .Net 2005. Basically I'm implementing a singleton class using double check locking pattern. I have read many arcticles about how this pattern is not thread safe, but my problem is slightly different. Here is the pseudo code:

MyMutex.h/cpp

class MyMutex {

public:
...

Lock() {

...

}

Unlock() {

...

}
...
};

ThreadMutex.h/cpp
class ThreadMutex {

public:
ThreadMutex() {
...
_lock = new MyMutex();
...
}

Acquire() {
...
_lock->Lock();
...
}

Release() {
...
_lock->Unlock();
...
}

...
private:
MyMutex* _lock;
}

MyObject.h

class MyObject {

protected:
MyObject();
...

public:
static MyObject Instance()

private:
static MyObject* _instance;
}

MyObject.cpp

MyObject* MyObject::_instance = 0;

MyObject* MyObject::Instance() {

if (_instance != 0) { //line 1
return _instance;
}

static ThreadMutex _instanceLock; //line 2

_instanceLock.Acquire(); //line 3

if (_instance == 0) {

_instance = new MyObject();
}

_instanceLock.Release();

return _instance;
}

MyObject will get accessed during application starting up (by two static object constructor) from two threads (yes the application has two threads even before main is started, one is main thread, another thread is created by a static object constructor). About 1 of 50 times, the application will crash at MyMutex::Lock() because "this=0x00000000"

When I debug on Visual Studio .Net 2005, it seemed to me what happened is the following in the order:

1. Thread 1 enters Instance() method at Line 1

2. Thread 1 proceeds to Line 2 because _instance =0, and Thread1 will initialize static object _instanceLock

3. Before _instanceLock is fullly constructed by Thread 1, Thread 1 is preempted by Thread 2

4. Thread 2 enters Instance() method at Line 1

5. Thread 2 proceeds to Line 3 because _instance =0 and static object _instanceLock is considered to be initialized at this time (decided by some sort complier flag )

6. Thread2 calls _instanceLock.Acquire(), which calls _lock->Lock(), but _lock is null at this time, since _instanceLock hasn't been fully constructed.

7. Application crashes

I think this can explain why this crashes happens occasionally.

I try to fix this by changing the function scope static _instnaceLock to class scope static, but found _instanceLock is still null when the first thread is trying to call _instanceLock.Acquire() and which of course crashes the application. This is understandable since we don't know the order of static objects get initialized during application starting up.

My another try is to use MyMutex directly in the Instance() method, which decreases the chances that threads get preempted during the construction of the _instanceLock object. So far I haven't seen crash yet with this change. But I think it's still not totally safe.

Could anyone tell me if my understanding about this crash is correct And is there any good approach to solve this problem

Thanks a lot!

Of course, it's perfectly possible to get decent initialization of local statics with dynamic initialization. That there is a requirement in the C++ for what happens on concurrent threads is news to me. Anyway, the Generic C++ ABI solves the problem with a really trivial set of initialization guards. The bottom line here is, it is certainly not a good idea in VC, but that's not a standard requirement but just a crappy implementation. BTW: I'm not sure if that has been mentioned anywhere, but usually a worse problem with local statics is that cleanup will often run under the loader lock.

Anyway, it's probably a good idea to initialize things before spawning multiple threads. And that's definitely possible in a mixed executable as well (in fact, I'm very certain that native global initializers run before any static constructors in a mixed - or pure image). If that doesn't work for you, there's still static initialization which you can rely on. Along with interlocked access one can easily construct a trivial lock. Just take a look at the (_)Interlocked(Compare)Exchange function/intrinsic.

-hg

Thanks for everyone's reply!

I did read many articles about how double check lock pattern is not thread safe. That's something I wasn't aware of when I chose to use it in my code. But I still think the crash hasn't reached creating the singleton instance part yet, but in creating lock. My understand is this lock is a singleton itself per singleton object (the real singleton object we want to create).

So I think I have two problems to solve now, how to make creating lock thread safe, and how to make the instance thread safe under the circumstances that application is starting up, multiple threads have been created before main(), and multiple threads try to create instance object.

-CG

As I said before, initialization of local static is safe, so long as its static initialization. For instance, something like:

void foo() {
static int i = 0;
}

Typically, compilers will statically allocate an integer in the initialized data segment of the image. So void foo()::i is guruanteed to be initialized to 0 before any code is executed. In case you wonder, the compiler will emit i in a section called ".bss" which ends in the ".data" section of your EXE/DLL and is marked as uninitialized data (that is a bit misleading as it is actually zero-initialized). The Windows loader will read the headers of the image, create the in-memory sections and initialize the accordingly before passing control to the program.

Hence, it is safe to rely on i being 0 on program startup. That is different from dynamic initialization which may be delayed to the first call which is problematic in the presence of multiple threads.

Now, with that background you can easily construct a trivial lock. Something like this should do the trick (I'm too lazy to try it out now, but it should give you the basic idea):

void guarded_init_function() {
enum { uninitialized, initializing, initialization_complete };
static volatile long init_guard = uninitialized;

long state;

while ( ( state = InterlockedCompareExchange( &init_guard
, initializing
, uninitialized ) ) == initializing )
Sleep(50);

if (state == initialization_complete ) return;

try {
actual_init();
} catch ( ... ) {
InterlockedExchange( &init_guard, uninitialized );
throw;
}
InterlockedExchange( &init_guard, initialization_complete );
}

}

-hg