std::lock_guard causing undefined behavior

Question

Edit: As it seems, the problem was me not actually creating a local instance of a lock_guard, but merely an anonymous temporary, which got destroyed again immediately, as pointed out by the comments below.

Edit2: Enabling clang's thread sanitizer helps to pinpoint these kinds of problems at run-time. It can be enabled via

clang++ -std=c++14 -stdlib=libc++ -fsanitize=thread *.cpp -pthread

---

This is probably in some way a duplicate question, but I couldn't find anything, so if it really is duplicate I'm sorry. This should be a beginner question anyway.

I was playing around with a simple "Counter" class, say inline in file

Counter.hpp:

#ifndef CLASS_COUNTER_HPP_
#define CLASS_COUNTER_HPP_

#include <mutex>
#include <string>
#include <exception>

class Counter
{
     public:
          explicit Counter(std::size_t v = 0) : value_{v} {}

          std::size_t value() const noexcept { return value_; }

//          void increment() { ++value_; }        // not an atomic operation : ++value_ equals value_ = value_ + 1
                                                // --> 3 operations: read, add, assign
          void increment() noexcept
          {
               mutex_.lock();
               ++value_;
               mutex_.unlock();
          }

//          void decrement() noexcept
//          {
//               mutex_.lock();
//               --value_;                      // possible underflow
//               mutex_.unlock();
//          }

          void decrement()
          {
               std::lock_guard<std::mutex>{mutex_};
               if (value_ == 0)
               {
                    std::string message{"New Value ("+std::to_string(value_-1)+") too low, must be at least 0"};
                    throw std::logic_error{message};
               }
               --value_;
          }

     private:
          std::size_t value_;
          std::mutex mutex_;
};

#endif

In main.cpp a Counter instance is supposed to be incremented and decremented concurrently:

main.cpp:

#include <iostream>
#include <iomanip>
#include <array>
#include <thread>
#include <exception>

#include "Counter.hpp"

     int
main ()
{
     Counter counter{};
     std::array<std::thread,4> threads;
     auto operation = [&counter]()
     {
          for (std::size_t i = 0; i < 125; ++i)
               counter.increment();
     };
//     std::for_each(begin(threads),end(threads),[&operation](auto& val) { val = std::thread{operation}; });
     std::cout << "Incrementing Counter (" << std::setw(3) << counter.value() << ") concurrently...";
     for (auto& t : threads)
     {
          t = std::thread{operation};
     }

     for (auto& t : threads)
          t.join();
     std::cout << " new value == " << counter.value() << '\n';

     auto second_operation = [&counter]()
     {
          for (std::size_t i = 0; i < 125; ++i)
          {
               try
               {
                    counter.decrement();
               }
               catch(const std::exception& e)
               {
                    std::cerr << "\n***Exception while trying to decrement : " << e.what() << "***\n";
               }
          }
     };

     std::cout << "Decrementing Counter (" << std::setw(3) << counter.value() << ") concurrently...";
     for (auto& t : threads)
          t = std::thread{second_operation};
     for (auto& t : threads)
          t.join();
     std::cout << " new value == " << counter.value() << '\n';

     return 0;

The exception handling seems to work as it's supposed to, and the way I understand it std::lock_guard is supposed to guarantee unlocking a mutex once the lock_guard goes out of scope.

However it seems to be more complicated than that. While the incrementation correctly results in a final value of "500", the decrementation - which is supposed to result in "0" - doesn't work out. The result will be something between "0" and "16" or so.

If the timing is changed, for instance by using valgrind, it seems to work correctly every time.

I was able to pinpoint the problem to the use of std::lock_guard. If I define the decrement() function as this :

      void decrement() noexcept
      {
           mutex_.lock();
           --value_;                      // possible underflow
           mutex_.unlock();
      }

everything works out fine ( as long as there is no underflow). But once I make a simple change to:

      void decrement() noexcept
      {    
           std::lock_guard<std::mutex>{mutex_};
           --value_;                      // possible underflow
      }

the behavior is like I described above. I presume I did not really understand the behavior and use cases of std::lock_guard. I would really appreciate it if you could point me into the right direction!

The program compiles via clang++ -std=c++14 -stdlib=libc++ *.cpp -pthread.

Answer 1

std::lock_guard<std::mutex>{mutex_}; Does not create a local. It creates a temporary which is destroyed at the end of the statement. This means your value is not protected by the lock. The lock guard must be a local:

void decrement() noexcept
{    
   std::lock_guard<std::mutex> guard {mutex_};
   --value_;                      // possible underflow
}

Answer 2

The problem is that the line

std::lock_guard<std::mutex>{mutex_};

does not create a variable, but rather creates a temporary lock_guard object which gets destroyed again immediately. What you probably meant to write was:

std::lock_guard<std::mutex> guard{mutex_};

This creates a variable of type lock_guard, named guard, which gets destroyed when it leaves the scope (i.e. at the end of the function. Essentially, you forgot to name your variable.