5

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.

6
  • 4
    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. Apr 4, 2012 at 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 possible
    – je4d
    Apr 4, 2012 at 19:57
  • 2
    @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
    Apr 4, 2012 at 19:59
  • 1
    @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
    Apr 4, 2012 at 20:03
  • Use either push_back or access by index. Both together don't make any sense.
    – selalerer
    Apr 4, 2012 at 20:39

3 Answers 3

3

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.)

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.

1
  • 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. Apr 5, 2012 at 7: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

1
  • 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.) Apr 5, 2012 at 7:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.