The gotcha of the C++ temporaries that don’t destruct as eagerly as you thought

Raymond Chen

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.

7 comments

Discussion is closed. Login to edit/delete existing comments.


Newest
Newest
Popular
Oldest
  • Pavel Kostromitinov 0

    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.

    • Raymond ChenMicrosoft employee Author · Edited 0

      It is significantly more complex: Instead of a simple a->b = c it’s a.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); }); If calculate_times_toggled also calls info.apply() then you deadlock.

  • Neil Rashbrook 0

    After seeing auto lock = info.Lock(); return lock->name; my mind jumped straight to return info.Lock()->name; without trying to oversimplify it to return info->name;. Is this good or bad?

  • Markus Schaber 0

    If you want to see a more elegant, language integrated version of this kind of mechanism, check Rust: https://www.rust-lang.org/ 🙂

    • Gunnar Dalsnes 0

      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.

  • MGetz 0

    Fun fact: don’t use out_ptr 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 out_ptr 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 to the suspend. Thus the unique_ptr is always filled with a nullptr, because that’s what’s in the out_ptr when it’s destructed. While it would be nice to have some way to give a compiler diagnostic or static code analysis diagnostic I’m not sure how one would go about that.

Feedback usabilla icon