CODEWITHSUNDEEP
c++multithreadingboostdouble-checked-locking
afsantos
afsantos

Reputation: 5206

Double-checked Locking for Shared Pointers

Disclaimer: I come from a Java background, and, as such, I have no clue on how a lot of the internals of C++ (and associated libraries) work.

I have read enough to know that double-checked locking is evil, and a correct and safe implementation of a singleton pattern requires the appropriate tools.

I believe the following code may be unsafe, subject to compiler reordering and assignment of uninitialized objects, but I am not sure if I am missing something that I do not understand about the language.

typedef boost::shared_ptr<A> APtr;

APtr g_a;
boost::mutex g_a_mutex;
const APtr& A::instance()
{
  if (!g_a)
  {
    boost::mutex::scoped_lock lock(g_a_mutex);
    if (!g_a)
    {
      g_a = boost::make_shared<A>();
    }
  }

  return g_a;
}

I believe the actual code is compiled under C++03.

Is this implementation unsafe? If so, how?

Upvotes: 5

Views: 340

Answers (2)

SergeyA
SergeyA

Reputation: 62613

All of this is absolutelty unneccesary in modern C++. Singleton code should be as simple as this for anybody but dinosaurs:

A& A::instance() {
    static A a;
    return a;
}

100% thread safe in C++11 and up.

However, I have a mandatory statement to make: Singletons are evil antipattern and should be banned forever.

On thread safety of original code

Yes, provided code is unsafe. Since the reading is done outside the mutex, it is totally possible that the modifications to g_a itself would be visible, but not that modifications to the object g_a is pointing to. As a result, the thread will bypass mutex locking and return a pointer to non-constructed object.

Upvotes: 1

ecatmur
ecatmur

Reputation: 157484

Yes, the double-checked locking pattern is unsafe in archaic languages. I can't do any better than the explanation in What's wrong with this fix for double checked locking?:

The problem apparently is the line assigning instance -- the compiler is free to allocate the object and then assign the pointer to it, OR to set the pointer to where it will be allocated, then allocate it.

If you were using C++11 std::shared_ptr, you could use atomic_load and atomic_store on the shared pointer object, which are guaranteed to compose correctly with each other and with mutexes; but if you're using C++11 you may as well just use a dynamically-initialized static variable, which is guaranteed to be thread-safe.

Upvotes: 3

Related Questions