Forgetting to take a lock when updating variables is a common mistake. One way to make the mistake harder to make is to force the access to occur through some mechanism that proves that you possess the lock. Maybe something like this:
template<typename> struct LockableData;
namespace std
{
template<typename Data>
struct default_delete<LockableData<Data>>
{
void operator()(LockableData<Data>* p)
const noexcept { p->m.unlock(); }
};
}
template<typename Lockable>
struct [[nodiscard]] LockedData
{
LockedData(Lockable* l = nullptr) : l(l)
{ if (l) l->m.lock(); }
auto operator->() const noexcept
{ return std::addressof(l->data); }
private:
std::unique_ptr<Lockable> l;
};
template<typename Data>
struct LockableData
{
LockedData<LockableData> Lock() { return this; }
private:
friend struct LockedData<LockableData>;
friend struct std::default_delete<LockableData>;
std::mutex m;
Data data;
};
The idea here is that you declare some structure that holds the data you want to be protected by a common mutex. You can then wrap that data inside a LockableData. To access the data, you call Lock() to acquire the mutex and receive a LockedData object. You then access the structure through the LockedData object, and when the LockedData object destructs, it releases the mutex.
Using a std::unique_ptr with a custom deleter allows the LockedData object to be movable with the natural semantics. And marking the LockedData as [[nodiscard]] makes sure that you save the return value of Lock(); otherwise, it destructs immediately, and your lock accomplished nothing.
Here’s an example usage:
struct WidgetInfo
{
std::string name;
int times_toggled = 0;
};
class Widget
{
LockableData<WidgetInfo> info;
public:
void SetName(std::string name)
{
auto lock = info.Lock();
lock->name = name;
lock->times_toggled = 0;
}
std::string GetName()
{
auto lock = info.Lock();
return lock->name;
}
void Toggle()
{
{ // scope the lock
auto lock = info.Lock();
lock->times_toggled++;
}
FlipSwitchesRandomly();
}
};
One thing that’s slightly annoying here is that in many cases, we are locking around the access to a single member. One way to avoid having to create tiny scopes is to allow the -> operator to be used directly from the LockableData, so that it does a lock-access-unlock.
template<typename Data>
struct LockableData
{
LockedData<LockableData> Lock() { return this; }
auto operator->() { return Lock(); } // NEW!
private:
friend struct LockedData<LockableData>;
friend struct std::default_delete<LockableData>;
std::mutex m;
Data data;
};
class Widget
{
LockableData<WidgetInfo> info;
public:
void SetName(std::string name)
{
auto lock = info.Lock();
lock->name = name;
lock->times_toggled = 0;
}
std::string GetName()
{
return info->name; // lock-read-unlock
}
void Toggle()
{
info->times_toggled++; // lock-modify-unlock
FlipSwitchesRandomly();
}
};
This convenience -> operator makes single-member updates much easier, but it also comes with a catch:
void Toggle()
{
info->times_toggled = std::max(info->times_toggled, 10);
FlipSwitchesRandomly();
}
Do you see the problem here?
I mean, yes, there’s a race condition here if two threads toggle at the same time, but that’s not the problem I’m referring to. That would “merely” result in incorrect accounting.
The real problem is the double lock.
The -> operator returns a temporary LockedData object, and the rules for temporary objects in C++ are that they are destructed at the end of the full expression.
Therefore, the evaluation of the revised code goes like this:
// Evaluate right hand side
LockedData<WidgetInfo> lock1 = info.operator->();
int rhs = std::max(lock1->times_toggled, 10);
// Evaluate left hand side
LockedData<WidgetInfo> lock2 = info.operator->();
// Perform the assignment
lock2->times_toggled = rhs;
// Destruct temporaries in reverse order of construction
destruct lock2;
destruct rhs;
destruct lock1;
Do you see the problem yet?
Since the locks are temporaries, their lifetime extends to the end of the full expression. We’ve seen this problem before.¹ This means that the two info->times_toggled operations each create a separate LockedData object, and each one acquires the mutex.
The result is that we acquire the mutex lock twice, which is not allowed. In practice, this results in a deadlock and forces you to issue a public apology.
The convenience -> was a bit too convenient: If an object o is a smart pointer, people are accustomed to o->Something doing some work with o in order to produce the pointer that will be dereferenced in order to access the Something. What they are not accustomed to is the presence of work that occurs after the dereference to clean up.
In other words, people tend to expect that it’s okay to call the -> operator many times on the same object without consequence.
So take away that dangerous operator. People can still get the one-liner convenience, though. They just have to lock explicitly:
std::string GetName()
{
return info.Lock()->name;
}
Having to write out the word “Lock” also makes it easier to spot that you’re locking twice within the same expression.
void Toggle()
{
// suspicious double-lock - more likely to be spotted in code review
info.Lock()->times_toggled = std::max(info.Lock()->times_toggled, 10);
FlipSwitchesRandomly();
}
¹ It looks like C++23 has adopted the idea of the out_ptr temporary object which resets the pointer on destruction, in spite of the footguns that arise in certain usage patterns.
There are versions of mutexes which are quite happy with double locking, std::recursive_mutex as a prime example.
Yes, I know that it comes with additional overhead, but for less performance-sensitive applications its usage can be justified.
For me personally it also comes with additional benefit of easier debugging, since it contains id of owner thread.
Easy-peasy with no footguns or complexity, no?
std::synchronized_value
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rconc-mutex
It is significantly more complex: Instead of a simple
a->b = cit’sa.apply([&](auto& v) { return v = c; }). It also does not remove the footgun. Just moves it to another place:info.apply([&] (auto& v) { v.times_toggled = calculate_times_toggled(info); });Ifcalculate_times_toggledalso callsinfo.apply()then you deadlock.After seeing
auto lock = info.Lock(); return lock->name;my mind jumped straight toreturn info.Lock()->name;without trying to oversimplify it toreturn info->name;. Is this good or bad?If you want to see a more elegant, language integrated version of this kind of mechanism, check Rust: https://www.rust-lang.org/ 🙂
I guess any language that does not allow operator overloading (except for comparison) would be more elegant:-) Operator overloading is the best tool to shoot oneself in the feets.
Fun fact: don't use across a coroutine suspend. This should be intuitive it's a footgun... but I still managed to do it because I assumed compiler magic. There was no magic... just an empty pointer and a memory leak. This is because again is a temporary and doesn't live across the scope so its destructor is called before the value is emplaced because temporaries are destructed (reasonably if you think about it) prior...