I have a class that is accessed from multiple threads. Both of its getter and setter functions are guarded with locks.
Are the locks for the getter functions really needed? If so, why?
class foo {
public:
void setCount (int count) {
boost::lock_guard<boost::mutex> lg(mutex_);
count_ = count;
}
int count () {
boost::lock_guard<boost::mutex> lg(mutex_); // mutex needed?
return count_;
}
private:
boost::mutex mutex_;
int count_;
};
The only way you can get around having the lock is if you can convince yourself that the system will transfer the guarded variable atomicly in all cases. If you can't be sure of that for one reason or another, then you'll need the mutex.
For a simple type like an int, you may be able to convince yourself this is true, depending on architecture, and assuming that it's properly aligned for single-instruction transfer. For any type that's more complicated than this, you're going to have to have the lock.
If you don't have a mutex around the getter, and a thread is reading it while another thread is writing it, you'll get funny results.
Is the mutex really only protecting a single int? It makes a difference -- if it is a more complex datatype you definitely need locking.
But if it is just an int, and you are sure that int is an atomic type (i.e., the processor will not have to do two separate memory reads to load the int into a register), and you have benchmarked the performance and determined you need better performance, then you may consider dropping the lock from both the getter and the setter. If you do that, make sure to qualify the int as volatile. And write a comment explaining why you do not have mutex protection, and under what conditions you would need it if the class changes.
Also, beware that you don't have code like this:
void func(foo &f) {
int temp = f.count();
++temp;
f.setCount(temp);
}
That is not threadsafe, regardless of whether you use a mutex or not. If you need to do something like that, the mutex protection has to be outside the setter/getter functions.
The synchronization concern is already covered in other answers (specifically David Schwartz's).
There's another concern I don't see addressed, though: this is usually a bad design.
Consider David's example code, assuming we have a correctly-synchronized version of foo
{
foo j;
some_func(j);
while (j.count() == 0)
{
// do we still expect (j.count() == 0) here?
bar();
}
}
The code suggests that the while condition still holds in the body. That's how single-threaded code works, after all.
But of course, even if we correctly synchronize the implementation of a getter, the setter can still be called from another thread, between our while condition succeeding and the first instruction of the loop body executing.
So, if any logic in the loop body can't depend on the condition being true, what was the point of testing it?
Sometimes it makes perfect sense, such as
while (foo.shouldKeepRunning())
{
// foo event loop or something
}
where it's OK if our shouldKeepRunning state changes during the loop body, because we only need to test it periodically. However, if you're going to do something with count, you need a longer-lived lock, and an interface to support it:
{
auto guard = j.lock_guard();
while (j.count(guard) == 0) // prove to count that we're locked
{
// now we _know_ count is zero in the body
// (but bar should release and re-acquire the lock or that can never change)
bar(j);
}
} // guard goes out of scope and unlocks
in you case probably not, if your cpu is 32 bit, however if count is a complex object or cpu needs more than one instruction to update its value, then yes
The lock is necessary to serialize access to shared resource. In your specific case you might get away with just atomic integer operations but in general, for larger objects that require more then one bus transaction, you do need locks to guarantee that reader always sees a consistent object.
It depends on the exact implementation of the object being locked. However, in general you do not want someone modifying (setting?) an object while someone else is in the process of reading (getting?) it. The easiest way to prevent that is to have a reader lock it.
In more complicated setups the lock will be implemented in such a way that any number of folks can read at once, but nobody can write to it while anyone is reading, and nobody can read while a write is going on.
They are really needed.
Imagine if you have an instance of class foo that's completely local to some piece of code. And you have something like this:
{
foo j;
some_func(j); // this stashes a reference to j where another thread can find it
while (j.count() == 0)
bar();
}
Suppose the optimizer looks carefully at the code to bar and sees that it can't possibly modify j.count_. This allows the optimizer to rewrite the code as follows:
{
foo j;
some_func(j); // this stashes a reference to j where another thread can find it
if (j.count() == 0)
{
while (1)
bar();
}
}
Clearly this is a disaster. Another thread might call j.setCount(5) and the thread wouldn't exit to loop.
The compiler can prove that bar can't modify the return value of j.count(). If it was required to assume that another thread could modify every memory value it accesses, it could never stash anything in a register ever, which would clearly be an untenable situation.
So, yes, the lock is needed. Alternatively, you need to use some other construct that provides similar guarantees.
Do not ever write code that relies on compilers not being able to make any optimization that they are permitted to make unless you really have no other practical choice. I have seen this cause a lot of pain over the many years I've been programming. Optimizers today can do things that would have been considered absurdly implausible a decade ago and lots of code lasts longer than you expect.
