4

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?

afsantos
  • 5,178
  • 4
  • 30
  • 54
  • 1
    IMHO, reading as writing without lock is bad : whats append if someone write inside `g_a` when you start to read it ? nothing ensures you that a reading isn't interrupt by a process which will write `g_a`, and continues just after. Remove first `if(!g_a)` and it will be safe – Garf365 Mar 17 '16 at 14:44

2 Answers2

2

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.

Community
  • 1
  • 1
ecatmur
  • 152,476
  • 27
  • 293
  • 366
  • It's not only the initialzation code. It might be CPU cache playing tricks here. It might be an optimization in compiler, which does really funny things with order of execution, assignments to main memory, etc. – SergeyA Mar 17 '16 at 15:04
1

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.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
  • I have seen that suggestion elsewhere, but I do not own the code in question, I am just analysing it, and I believe the actual binaries are compiled under C++03. I will get that cleared up. I wholeheartedly agree that singletons should be banned, however. – afsantos Mar 17 '16 at 14:51
  • @afsantos, the code is unsafe. I have explained why. – SergeyA Mar 17 '16 at 14:56
  • Exception to the "Singletons are evil anti pattern" rule -- if the returned singleton object is immutable, then it's okay, since there can be no thread-safety problems if the object is never modified – Jeremy Friesner Mar 17 '16 at 16:45
  • @JeremyFriesner, first, antipatterness of singletons has nothing to do with thread-safety. It is an antipattern even in single-threaded applications. Second, when we talk about singleton thread-safety, we usually refer to it's creation, not the thread-safety of "underlying" object. Because of that, being immutable is irrelevant for singleton thread-safety concept. – SergeyA Mar 17 '16 at 16:48
  • @SergeyA the alternative is to have each caller construct and use its own separate (yet 100%-identical) object instead, which uses additional memory and CPU time, for no obvious benefit in the immutable-object case. Regarding thread-safety of the object's creation, your post says your example is "100% thread safe in C++11 and up", so it seems that is not an issue. – Jeremy Friesner Mar 17 '16 at 16:57
  • @JeremyFriesner, no, alternative is to use a const global object. – SergeyA Mar 17 '16 at 16:58
  • Sure, that works too. Wrapping it in an accessor function gives a bit more flexibility/encapsulation, though. – Jeremy Friesner Mar 18 '16 at 05:58