Any comments to this (hopefully) 100% safe double-check-locking replacement for singletons.

Question

As Scott Meyers and Andrei Alexandrescu outlined in this article the simple try to implement the double-check locking implementation is unsafe in C++ specifically and in general on multi-processor systems without using memory barriers.

I was thinking a little bit about that and came to a solution that avoids using memory barriers and should also work 100% safe in C++. The trick is to store a copy of the pointer to the instance thread-local so each thread has to acquire the lock for the first times it access the singleton.

Here is a little sample code (syntax not checked; I used pthread but all other threading libs could be used):

class Foo
 { 
 private:
   Helper *helper;
   pthread_key_t localHelper;
   pthread_mutex_t mutex;
 public:
   Foo()
     : helper(NULL)
   {
     pthread_key_create(&localHelper, NULL);
     pthread_mutex_init(&mutex);
   }
   ~Foo()
   {
     pthread_key_delete(&localHelper);
     pthread_mutex_destroy(&mutex);
   }
   Helper *getHelper()
   {
     Helper *res = pthread_getspecific(localHelper);
     if (res == NULL)
     {
       pthread_mutex_lock(&mutex);
       if (helper == NULL)
       {
         helper = new Helper();
       }
       res = helper;
       pthread_mutex_unlock(&mutex);
       pthread_setspecific(localHelper, res);
     }
     return res;
   }
 };

What are your comments/opinions?

Do you find any flaws in the idea or the implementation?

EDIT:

Helper is the type of the singleton object (I know the name is not the bet...I took it from the Java examples in the Wikipedia article about DCLP). Foo is the Singleton container.

EDIT 2:

Because it seems to be a little bit misunderstanding that Foo is not a static class and how it is used, here an example of the usage:

static Foo foo;

.
.
.

foo.getHelper()->doSomething();

.
.
.

The reason that Foo's members are not static is simply that I was able to create/destroy the mutex and the TLS in the constructor/destructor. If a RAII version of a C++ mutex / TLS class is used Foo can easily be switched to be static.

Answer 1

You seem to be calling:

pthread_mutex_init(&mutex);

in the Helper() constructor. But that constructor is itself called in the function getHelper() (which should be static, I think) which uses the mutex. So the mutex appears to be initialised twice or not at all.

I find the code very confusing, I must say. Double-checked locking is not that complex. Why don't you start again, and this time create a Mutex class, which does the initialisation, and uses RAI to release the underlying pthread mutex. Then use this Mutex class to implement your locking.

Answer 2

This isn't the double-checked locking pattern. Most of the potential thread safety issues of the pattern are due to the fact the the a common state is read outside of a mutually exclusive lock, and then re-checked inside it.

What you are doing is checking a thread local data item, and then checking the common state inside a lock. This is more like a standard single check singleton pattern with a thread local cached value optimization.

To a casual glance it does look safe, though.

Answer 3

Looks interesting! Clever use of thread-local storage to reduce contention.

But I wonder if this is really different from the problematic approach outlined by Meyers/Alexandrescu...?

Say you have two threads for which the singleton is uninitialized (e.g. thread local slot is empty) and they run getHelper in parallel.

Won't they get into the same race over the helper member? You're still calling operator new and you're still assigning that to a member, so the risk of rogue reordering is still there, right?

EDIT: Ah, I see now. The lock is taken around the NULL-check, so it should be safe. The thread-local replaces the "first" NULL-check of the DCLP.

Answer 4

Are you trying to make a thread-safe singleton or, implement Meyers & Andrescu's tips? The most straightforward thing is to use a

pthread_once

as is done below. Obviously you're not going for lock-free speed or anything, creating & destroying mutexes as you are so, the code may as well be simple and clear - this is the kind of thing pthread_once was made for. Note, the Helper ptr is static, otherwise it'd be harder to guarantee that there's only one Helper object:

// Foo.h

#include <pthread.h>

class Helper {};

class Foo
{
private:
   static Helper* s_pTheHelper;
   static ::pthread_once_t once_control;

private:
   static void createHelper() { s_pTheHelper = new Helper(); }

public:
   Foo()
   { // stuff
   }

   ~Foo()
   { // stuff
   }

   static Helper* getInstance()
   {
      ::pthread_once(&once_control, Foo::createHelper);
      return s_pTheHelper;
   }
};

// Foo.cpp
// ..
Helper* Foo::s_pTheHelper = NULL;
::pthread_once_t Foo::once_control = PTHREAD_ONCE_INIT;
// ..

Answer 5

Obviously a few people are misunderstanding your intent/solution. Something to think about.

I think the real question to be asked is this:

Is calling pthread_getspecific() cheaper than a memory barrier?