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.
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
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
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
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.)
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.
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
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
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
A
). For example, you can't simply accesssharedResource[id]
without actually having done something to resizesharedResource
to containid + 1
elements. And unlessA
contains a member functionpush_back
, the code shouldn't even compile - James Kanze 2012-04-04 18:51