{"id":107248,"date":"2022-10-05T07:00:00","date_gmt":"2022-10-05T14:00:00","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/oldnewthing\/?p=107248"},"modified":"2022-10-05T15:35:00","modified_gmt":"2022-10-05T22:35:00","slug":"20221005-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20221005-00\/?p=107248","title":{"rendered":"The gotcha of the C++ temporaries that don’t destruct as eagerly as you thought"},"content":{"rendered":"

Forgetting to take a lock when updating variables is a common mistake. One way to make the mistake harder to make is to force<\/i> the access to occur through some mechanism that proves that you possess the lock. Maybe something like this:<\/p>\n

template<typename> struct LockableData;\r\n\r\nnamespace std\r\n{\r\n    template<typename Data>\r\n    struct default_delete<LockableData<Data>>\r\n    {\r\n        void operator()(LockableData<Data>* p)\r\n        const noexcept { p->m.unlock(); }\r\n    };\r\n}\r\n\r\ntemplate<typename Lockable>\r\nstruct [[nodiscard]] LockedData\r\n{\r\n    LockedData(Lockable* l = nullptr) : l(l)\r\n    { if (l) l->m.lock(); }\r\n\r\n    auto operator->() const noexcept\r\n    { return std::addressof(l->data); }\r\n\r\nprivate:\r\n    std::unique_ptr<Lockable> l;\r\n};\r\n\r\ntemplate<typename Data>\r\nstruct LockableData\r\n{\r\n    LockedData<LockableData> Lock() { return this; }\r\n\r\nprivate:\r\n    friend struct LockedData<LockableData>;\r\n    friend struct std::default_delete<LockableData>;\r\n\r\n    std::mutex m;\r\n    Data data;\r\n};\r\n<\/pre>\n
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<\/code>. To access the data, you call Lock()<\/code> to acquire the mutex and receive a LockedData<\/code> object. You then access the structure through the LockedData<\/code> object, and when the LockedData<\/code> object destructs, it releases the mutex.<\/p>\n
Using a std::unique_ptr<\/code> with a custom deleter allows the LockedData<\/code> object to be movable with the natural semantics. And marking the LockedData<\/code> as [[nodiscard]]<\/code> makes sure that you save the return value of Lock()<\/code>; otherwise, it destructs immediately, and your lock accomplished nothing.<\/p>\n
Here’s an example usage:<\/p>\n
struct WidgetInfo\r\n{\r\n    std::string name;\r\n    int times_toggled = 0;\r\n};\r\n\r\nclass Widget\r\n{\r\n    LockableData<WidgetInfo> info;\r\n\r\npublic:\r\n    void SetName(std::string name)\r\n    {\r\n        auto lock = info.Lock();\r\n        lock->name = name;\r\n        lock->times_toggled = 0;\r\n    }\r\n\r\n    std::string GetName()\r\n    {\r\n        auto lock = info.Lock();\r\n        return lock->name;\r\n    }\r\n\r\n    void Toggle()\r\n    {\r\n        { \/\/ scope the lock\r\n            auto lock = info.Lock();\r\n            lock->times_toggled++;\r\n        }\r\n        FlipSwitchesRandomly();\r\n    }\r\n};\r\n<\/pre>\n
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 -><\/code> operator to be used directly from the LockableData<\/code>, so that it does a lock-access-unlock.<\/p>\n
template<typename Data>\r\nstruct LockableData\r\n{\r\n    LockedData<LockableData> Lock() { return this; }\r\n    auto operator->() { return Lock(); } \/\/ NEW!<\/span>\r\n\r\nprivate:\r\n    friend struct LockedData<LockableData>;\r\n    friend struct std::default_delete<LockableData>;\r\n\r\n    std::mutex m;\r\n    Data data;\r\n};\r\n\r\nclass Widget\r\n{\r\n    LockableData<WidgetInfo> info;\r\n\r\npublic:\r\n    void SetName(std::string name)\r\n    {\r\n        auto lock = info.Lock();\r\n        lock->name = name;\r\n        lock->times_toggled = 0;\r\n    }\r\n\r\n    std::string GetName()\r\n    {\r\n        return info->name; \/\/ lock-read-unlock<\/span>\r\n    }\r\n\r\n    void Toggle()\r\n    {\r\n        info->times_toggled++; \/\/ lock-modify-unlock<\/span>\r\n        FlipSwitchesRandomly();\r\n    }\r\n};\r\n<\/pre>\n
This convenience -><\/code> operator makes single-member updates much easier, but it also comes with a catch:<\/p>\n
    void Toggle()\r\n    {\r\n        info->times_toggled = std::max(info->times_toggled, 10);<\/span>\r\n        FlipSwitchesRandomly();\r\n    }\r\n<\/pre>\n
Do you see the problem here?<\/p>\n
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.<\/p>\n
The real problem is the double lock.<\/p>\n
The -><\/code> operator returns a temporary LockedData<\/code> object, and the rules for temporary objects in C++ are that they are destructed at the end of the full expression<\/i>.<\/p>\n
Therefore, the evaluation of the revised code goes like this:<\/p>\n
    \/\/ Evaluate right hand side\r\n    LockedData<WidgetInfo> lock1 = info.operator->();\r\n    int rhs = std::max(lock1->times_toggled, 10);\r\n\r\n    \/\/ Evaluate left hand side\r\n    LockedData<WidgetInfo> lock2 = info.operator->();\r\n\r\n    \/\/ Perform the assignment\r\n    lock2->times_toggled = rhs;\r\n\r\n    \/\/ Destruct temporaries in reverse order of construction\r\n    destruct lock2;\r\n    destruct rhs;\r\n    destruct lock1;\r\n<\/pre>\n
Do you see the problem yet?<\/p>\n
Since the locks are temporaries, their lifetime extends to the end of the full expression.  We’ve seen this problem before<\/a>.\u00b9 This means that the two info->times_toggled<\/code> operations each create a separate LockedData<\/code> object, and each one acquires the mutex.<\/p>\n
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<\/a>.<\/p>\n
The convenience -><\/code> was a bit too convenient<\/i>: If an object o<\/code> is a smart pointer, people are accustomed to o->Something<\/code> doing some work with o<\/code> in order to produce the pointer that will be dereferenced in order to access the Something<\/code>. What they are not accustomed to is the presence of work that occurs after the dereference to clean up.<\/p>\n
In other words, people tend to expect that it’s okay to call the -><\/code> operator many times on the same object without consequence.<\/p>\n
So take away that dangerous operator. People can still get the one-liner convenience, though. They just have to lock explicitly:<\/p>\n
    std::string GetName()\r\n    {\r\n        return info.Lock()->name;\r\n    }\r\n<\/pre>\n
Having to write out the word “Lock” also makes it easier to spot that you’re locking twice within the same expression.<\/p>\n
    void Toggle()\r\n    {\r\n        \/\/ suspicious double-lock - more likely to be spotted in code review\r\n        info.Lock()->times_toggled = std::max(info.Lock()->times_toggled, 10);\r\n        FlipSwitchesRandomly();\r\n    }\r\n<\/pre>\n
\u00b9 It looks like C++23 has adopted the idea of the out_ptr<\/code> temporary object which resets the pointer on destruction, in spite of the  footguns<\/a> that arise in certain usage patterns.<\/p>\n","protected":false},"excerpt":{"rendered":"
You have to look for the end of the full expression.<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-107248","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
You have to look for the end of the full expression.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/107248","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=107248"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/107248\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=107248"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=107248"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=107248"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}