Re: Please disprove this Double-Checked Locking "fix"

Hi,

    Recently I've been reading up on "Double-Checked Locking" in C++
and how it's often implemented imperfectly. The Article "C++ and the
Perils of Double-Checked Locking" by Scott Meyers and Andrei
Alexandrescu ( http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf
) provides a good overview of how it's usually done and why it's often
inadequate.

    I won't go into details here, but the article states how this code:

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

and its variants aren't completely thread safe.

    Now, I've been thinking about how to make the code thread-safe, and
a few days ago I came up with a couple of solutions. As I implemented
them, I realized that these variants of mine also had problems.

    One of the "solutions" I came up with was this:

// Assign the singleton object to a temp pointer:
Singleton* Singleton::instance() {
   if (pInstance == 0) {
     Lock lock;
     if (pInstance == 0) {
       Singleton* temp = new Singleton; // initialize to temp
       pInstance = temp; // assign temp to pInstance
     }
   }
   return pInstance;
}

Now, this approach isn't new (and is in fact covered in Meyer's and
Alexandrescu's article). The immediate problem is that the compiler
can optimize away the temporary variable, so there's no guarantee that
pInstance won't get the memory before it is properly initialized.

    So I thought about making sure pInstance was unset in the
constructor, like this:

Singleton::Singleton() {
   // initializing member data
   assert( pInstance == 0 );
}

This way, the assert() statement ensures that pInstance will stay NULL
until the constructor is finished.

    BUT! I realized that this solution was flawed, because the
compiler can inline the constructor into the ::instance() method, like
this:

Singleton* Singleton::instance() {
   if (pInstance == 0) {
     Lock lock;
     if (pInstance == 0) {
       Singleton* temp = operator new(sizeof(Singleton)); //
initialize to temp
       new (temp) Singleton;
       // initializing member data
       assert( pInstance == 0 );
       pInstance = temp; // assign temp to pInstance
     }
   }
   return pInstance;
}

and further rearrange the lines like this:

Singleton* Singleton::instance() {
   if (pInstance == 0) {
     Lock lock;
     if (pInstance == 0) {
       Singleton* temp = operator new(sizeof(Singleton)); //
initialize to temp
       new (temp) Singleton;
       assert( pInstance == 0 );
       pInstance = temp; // assign temp to pInstance
       // initializing member data
     }
   }
   return pInstance;
}

(The compiler is allowed to do this, I think, because the compiler can
reorder code as long as it's not detectable in a single-threaded
program.)

So I realized pInstance still has the possibility of being assigned
memory before it is properly initialized.

    Then I thought: What if I included a second lock to make sure that
the constuctor is finished before pInstance is set? So I tried this:

Singleton* Singleton::instance() {
   if (pInstance == 0) {
     Lock lock;
     if (pInstance == 0) {
       Singleton* temp = new Singleton; // initialize to temp
       secondLock.lock();
       pInstance = temp; // assign temp to pInstance
       secondLock.unlock();
       }
     }
   }
   return pInstance;
}

Singleton::Singleton() {
   secondLock.lock();
   // initializing member data
   secondLock.unlock();
}

But eventually I realized that the order of the locks is not
guaranteed, that pInstance could still get assigned before the
constructor gets entered!

    So I shot down my own two solutions. However, I kept thinking of
variants, and I started to wonder: What if I combined the two
variants? In other words, I'd take advantage of both assert() and a
second lock, like this:

Singleton* Singleton::instance() {
   if (pInstance == 0) {
     Lock lock;
     if (pInstance == 0) {
       Singleton* temp = new Singleton; // initialize to temp
       secondLock.lock();
       pInstance = temp; // assign temp to pInstance
       secondLock.unlock();
       }
     }
   }
   return pInstance;
}

Singleton::Singleton() {
   secondLock.lock();
   // initializing member data
   assert( pInstance == 0 );
   secondLock.unlock();
}

In this way, the second lock guarantees that pInstance is not assigned
to inside the constructor code, and the assert() statement ensures
that the constructor code executes before the code that assigns to
pInstance.

    This looks good as far as I can tell, but I'm thinking it's too
good to be true.

    Since I'm a bit skeptical about this last solution, could someone
poke holes in it? (I'm eager to see if I really did find a solid
solution, or if it's just another pipe dream.)