What is wrong with std::lock_guard

Question

I have simple code: first thread pushes std::strings to the std::list, and second thread pops std::strings from this std::list. All std::lists operations are protected with std::mutex m. This code permanently prints error to console: "Error: lst.begin() == lst.end()".

If I replace std::lock_guard with construction m.lock() and m.unlock() the code begins work correctly. What is wrong with std::lock_guard?

#include <iostream>
#include <thread>
#include <mutex>
#include <list>
#include <string>

std::mutex m;
std::list<std::string> lst;

void f2()
{
    for (int i = 0; i < 5000; ++i)
    {
        std::lock_guard<std::mutex> { m };
        lst.push_back(std::to_string(i));
    }

    m.lock();
    lst.push_back("-1"); // last list's element
    m.unlock();
}

void f1()
{
    std::string str;

    while (true)
    {
        m.lock();
        if (!lst.empty())
        {
            if (lst.begin() == lst.end())
            {
                std::cerr << "Error: lst.begin() == lst.end()" << std::endl;
            }
            str = lst.front();
            lst.pop_front();
            m.unlock();
            if (str == "-1")
            {
                break;
            }
        }
        else
        {
            m.unlock();
            std::this_thread::yield();
        }
    }
}

// tested in MSVS2017
int main()
{
    std::thread tf2{ f2 };
    f1();
    tf2.join();
}

[P0577](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0577r1.pdf) proposes to avoid this by repurposing the `register` keyword to mean 'associate the following anonymous object(s) with this entire scope'. — underscore_d, Jan 01 '19 at 11:55

Werner Henze · Answer 1 · 2019-01-03T14:40:52.193

22

You did not obey CppCoreGuidelines CP.44: Remember to name your lock_guards and unique_locks :).

In

for (int i = 0; i < 5000; ++i)
{
    std::lock_guard<std::mutex> { m };
    lst.push_back(std::to_string(i));
}

you are only creating a temporary std::lock_guard object which is created and destroyed immediately. You need to name the object like in

{
    std::lock_guard<std::mutex> lg{ m };
    lst.push_back(std::to_string(i));
}

so that the lock guard lives until the end of the block.

And as you already recognized (CppCoreGuidelines):

Use RAII lock guards (lock_guard, unique_lock, shared_lock), never call mutex.lock and mutex.unlock directly (RAII)

If you are using Microsoft Visual Studio, I recommend using the code analysis and activating at least the Microsoft Native Recommended Rules. If you do this you will get a compiler analysis warning.

warning C26441: Guard objects must be named (cp.44).

edited Jan 03 '19 at 14:40

answered Dec 31 '18 at 11:14

Werner Henze

16,404
12
44
69

@Igorrr37 I updated my answer, this is even a guideline in the CppCoreGuidelines. If my answer worked for you, it would be nice if you accept it. – Werner Henze Dec 31 '18 at 11:24
3

[This is such an easy mistake to make, unfortunately.](https://stackoverflow.com/a/16189942/560648) – Lightness Races in Orbit Dec 31 '18 at 14:17
There is actually a good reason to have unnamed temporary `lock_guard`s and `unique_lock`s: If only a single expression needs to execute with the lock held: `std::lock_guard{m}, do_locked(a,b,c);`. – Deduplicator Dec 31 '18 at 15:57
3

@Deduplicator I question how useful it really is, merely so you can fit everything on one line, while having unusual syntax and requiring a rewrite if you want something else covered by the lock (which can be forgotten and leave you with an immediately dying lock as in the OP, is hostile to version control diffing, etc.). – underscore_d Jan 02 '19 at 19:21
1

It's a shame you can't `[[nodiscard]]` a constructor. – Raymond Chen Jan 03 '19 at 16:16
@RaymondChen Starting with C++20 `[[nodiscard]]` can be used on constructors :). But it looks like it is not yet used on `unique_lock` constructor :(. – Werner Henze May 10 '20 at 19:32

What is wrong with std::lock_guard

1 Answers1