Double-checked locking around value going to null?

Question

Is this thread safe:

class X;

class Once {
 public:
  Once(X* x) : x_(x) {}

  X* Get() {
    if (!x_) return NULL;
    // all dirty reads end up here.

    // This could be any type of scoped lock...
    some_scoped_lock lock(m);

    // if (!x_) return x_;  // omitted because it's a no op

    X* ret(x_);  // might get NULL if we waited for lock
    x_ = NULL;   // idempotent

    return ret;
  }

 private:
  X *x_;
  some_kind_of_mutex m;

  // Boilerplate to make all other constructors and default function private
  ....
}

(edit: i'm interested in both c++11 and older versions)

As I understand it the problem with Double-checked locking is that in some memory models the write to the guarded var can occur and become visible to early. I think the above code doesn't have this issue because there is no precondition for the validity of the new value. I think the only requirement for this to be correct code is that all reads under the lock must be clean with respect to writes in the constructor and writes under the lock.

Update: OK, it seems that this invokes the "undefined behavior" pitfall and therefor can legally do anything, including drain your bank account. That said, are there any cases where it will misbehave?

In double-checked locking, there is double-checking. I only see one check. — David Heffernan, Apr 20 '12 at 16:36
@DavidHeffernan: The second check is omitted because it's a no op. — BCS, Apr 20 '12 at 16:39
Well, what happens if some non-`NULL` value of `x_` is stale in the current thread? — Vlad, Apr 20 '12 at 16:39
@Vlad: the `!x_` check reads a stale value and results in a redundant write to `x_` setting it to `NULL` a second time... I think. — BCS, Apr 20 '12 at 16:42
There are other issues with caching on multi-threads that you have not considered. C++11 has extended the language to cope with this go look at the new primatives provided by the language. — Martin York, Apr 20 '12 at 16:49
@Loki: could you please elaborate more on the caching issues? I cannot see them at the moment. — Vlad, Apr 20 '12 at 16:52
@LokiAstari C++ didn't define thread safety, so you had to fall back on other standards. The C++ rules are exactly those of Posix; it was undefined behavior under Posix as well. — James Kanze, Apr 20 '12 at 17:12
@BCS: Pre C++11 you are out of luck. Though some compilers have propriety extensions to language features (like MS compiler by specifying the member as volatile). — Martin York, Apr 20 '12 at 17:47
@Vlad: Myers and Alexandrescu explain it very well. http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf — Martin York, Apr 20 '12 at 17:51

score 2 · Answer 1 · answered Apr 20 '12 at 17:04

2

According to the C++11 standard, the behavior is undefined, because there is a data race. In more detail: Thread A writes to x_ in the line x_ = NULL and Thread B reads from x_ in the line with if (!x_) return NULL. These two operations are not sequenced. That means there is a data race, and that implies undefined behavior.

You should use an atomic type to avoid the data race. This is very simple in your example. However, I think the question was more general. Nevertheless:

struct Once
{
    std::atomic<X*> _x;
    explicit Once(X* x) : _x{x} {}
    X* Get()
    {
        return _x.exchange(nullptr, std::memory_order::memory_order_relaxed);
    }
};

answered Apr 20 '12 at 17:04

nosid

48,932
13
112
139

you ought to declare `_x` as private – Vlad Apr 20 '12 at 17:13
3

1.11p21 The execution of a program contains a data race if it contains two conflicting actions in different threads, at least one of which is not atomic, and neither happens before the other.Now, the question is whether those two operation are *conflicting* (the standard does not seem to provide a definition for conflicting) and whether they are *atomic*. Reads and writes of pointers are *atomic* in many architectures (including intel). I am not so sure that the code is actually *undefined behavior* – David Rodríguez - dribeas Apr 20 '12 at 18:01
I guess that begs the question: are identical writes conflicting actions? – BCS Apr 20 '12 at 18:08
In C++11, reads and writes to pointers are _not_ atomic. You should use `std::atomic`, and if the hardware supports atomic operations without additional memory barriers, there is no overhead in using `std::atomic` in the above example. _Identical_ writes to the same memory location are also data races. I recommend an introduction to the *C++11 Memory Model*, e.g. [Threads and Shared Variables in C++11](http://channel9.msdn.com/Events/GoingNative/GoingNative-2012/Threads-and-Shared-Variables-in-C-11) by Hans Boehm. – nosid Apr 20 '12 at 18:23
@DavidRodríguez-dribeas "Atomic" can have two different meanings. Reads and writes to pointers are _not_ atomic according to the meaning used in the standard; for an operation to be atomic, it must use one of the designated library functions. – James Kanze Apr 23 '12 at 08:11

score 1 · Answer 2 · answered Apr 20 '12 at 16:55

1

Not exactly sure, but I think you have to insert a memory barrier/lock in the constructor.

Imagine that the memory which will be allocated for x_ had the value of NULL before constructor, and this value is stale on some thread. Imagine that this thread tries to call Get() -- it will erroneously get NULL back due to the early return w/o memory barriers.

answered Apr 20 '12 at 16:55

Vlad

35,022
6
77
199

I think Vlad is technically correct. Thread safety of object creation is compiler dependent, although it is probably safe on virtually all modern compilers. http://stackoverflow.com/questions/796099/c-new-operator-thread-safety-in-linux-and-gcc-4 – patros Apr 20 '12 at 17:07
@patros: I think, although the constructor can be thread-safe, it doesn't set any memory barriers, so `x_` might still have a stale value after all. – Vlad Apr 20 '12 at 20:31

Double-checked locking around value going to null?

2 Answers2

Linked