Thread-safe initialization of atomic variable in C++

Go To StackoverFlow.com

4

Consider the following C++11 code where class B is instantiated and used by multiple threads. Because B modifies a shared vector, I have to lock access to it in the ctor and member function foo of B. To initialize the member variable id I use a counter that is an atomic variable because I access it from multiple threads.

struct A {
  A(size_t id, std::string const& sig) : id{id}, signature{sig} {}
private:
  size_t id;
  std::string signature;
};
namespace N {
  std::atomic<size_t> counter{0};
  typedef std::vector<A> As;
  std::vector<As> sharedResource;
  std::mutex barrier;

  struct B {
    B() : id(++counter) {
      std::lock_guard<std::mutex> lock(barrier);
      sharedResource.push_back(As{});
      sharedResource[id].push_back(A("B()", id));
    }
    void foo() {
      std::lock_guard<std::mutex> lock(barrier);
      sharedResource[id].push_back(A("foo()", id));
    }
  private:
    const size_t id;
  };
}

Unfortunately, this code contains a race condition and does not work like this (sometimes the ctor and foo() do not use the same id). If I move the initialization of id to the ctor body which is locked by a mutex, it works:

struct B {
  B() {
    std::lock_guard<std::mutex> lock(barrier);
    id = ++counter; // counter does not have to be an atomic variable and id cannot be const anymore
    sharedResource.push_back(As{});
    sharedResource[id].push_back(A("B()", id));
  }
};

Can you please help me understanding why the latter example works (is it because it does not use the same mutex?)? Is there a safe way to initialize id in the initializer list of B without locking it in the body of the ctor? My requirements are that id must be const and that the initialization of id takes place in the initializer list.

2012-04-04 18:44
by michael
Can you post the actual code which is causing the problem. The code you posed doesn't make sense (at least in the absence of a definition of A). For example, you can't simply access sharedResource[id] without actually having done something to resize sharedResource to contain id + 1 elements. And unless A contains a member function push_back, the code shouldn't even compile - James Kanze 2012-04-04 18:51
@JamesKanze why should A need a push_back member? I see only a (const char*,size_t) constructor and a move/copy constructor in use. OP: Please make this an SSCCE if possibl - je4d 2012-04-04 19:57
@je4d : sharedResource is a std::vector<A>, so sharedResource[id] returns an A& and sharedResource[id].push_back(...) is thus calling A::push_back - ildjarn 2012-04-04 19:59
@ildjarn ah yes, I scanned it too quickly and assumed that because an A was being pushed, it wasn't being pushed into an A because that'd make little sense for the conventional semantics of push_back. I expect the code's there's not what the OP actually intended to write - je4d 2012-04-04 20:03
Use either push_back or access by index. Both together don't make any sense - selalerer 2012-04-04 20:39
Added more context to make the example self contained - michael 2012-04-04 22:01


2

First, there's still a fundamental logic problem in the posted code. You use ++ counter as id. Consider the very first creation of B, in a single thread. B will have id == 1; after the push_back of sharedResource, you will have sharedResource.size() == 1, and the only legal index for accessing it will be 0.

In addition, there's a clear race condition in the code. Even if you correct the above problem (initializing id with counter ++), suppose that both counter and sharedResource.size() are currently 0; you've just initialized. Thread one enters the constructor of B, increments counter, so:

counter == 1
sharedResource.size() == 0

It is then interrupted by thread 2 (before it acquires the mutex), which also increments counter (to 2), and uses its previous value (1) as id. After the push_back in thread 2, however, we have only sharedResource.size() == 1, and the only legal index is 0.

In practice, I would avoid two separate variables (counter and sharedResource.size()) which should have the same value. From experience: two things that should be the same won't be—the only time redundant information should be used is when it is used for control; i.e. at some point, you have an assert( id == sharedResource.size() ), or something similar. I'd use something like:

B::B()
{
    std::lock_guard<std::mutex> lock( barrier );
    id = sharedResource.size();
    sharedResource.push_back( As() );
    //  ...
}

Or if you want to make id const:

struct B
{
    static int getNewId()
    {
        std::lock_guard<std::mutex> lock( barrier );
        int results = sharedResource.size();
        sharedResource.push_back( As() );
        return results;
    }

    B::B() : id( getNewId() )
    {
        std::lock_guard<std::mutex> lock( barrier );
        //  ...
    }
};

(Note that this requires acquiring the mutex twice. Alternatively, you could pass the additional information necessary to complete updating sharedResource to getNewId(), and have it do the whole job.)

2012-04-05 07:50
by James Kanze


1

When an object is being initialized, it should be owned by a single thread. Then when it is done being initialized, it is made shared.

If there is such a thing as thread-safe initialization, it means ensuring that an object has not become accessible to other threads before being initialized.

Of course, we can discuss thread-safe assignment of an atomic variable. Assignment is different from initialization.

2012-04-05 01:20
by Kaz
In his example, the object being initialized (of type B) is only accessible from a single thread (I would suppose). His problem is that the constructor of that object uses global resources - James Kanze 2012-04-05 07:51


0

You are in the sub-constructor list initializing the vector. This is not really an atomic operation. so in a multi-threaded system you could get hit by two threads at the same time. This is changing what id is. welcome to thread safety 101!

moving the initialization into the constructor surrounded by the lock makes it so only one thread can access and set the vector.

The other way to fix this would be to move this into a singelton pattern. But then you are paying for the lock every time you get the object.

Now you can get into things like double checked locking :)

http://en.wikipedia.org/wiki/Double-checked_locking

2012-04-05 01:45
by nate_weldon
Double checked locking is a clear anti-pattern; it is extremely difficult to get right, and never really necessary. In his case, if he were to make sharedResource a singleton, the static instance function can acquire the lock, and return an std::shared_ptr whose "destructor" object releases it. (This should be a standard pattern for mutable singletons in a multithreaded environment. - James Kanze 2012-04-05 07:56
Ads