CODEWITHSUNDEEP
c++c++11thread-safetysegmentation-faultmutex
stefan
stefan

Reputation: 10355

Possibility of segmentation fault with mutex in class with multithreading

With multiple threads (std::async) sharing an instance of the following class through a shared_ptr, is it possible to get a segmentation fault in this part of the code? If my understanding of std::mutex is correct, mutex.lock() causes all other threads trying to call mutex.lock() to block until mutex.unlock() is called, thus access to the vector should happen purely sequentially. Am I missing something here? If not, is there a better way of designing such a class (maybe with a std::atomic_flag)?

#include <mutex>
#include <vector>
class Foo
{
   private:
      std::mutex mutex;
      std::vector<int> values;
   public:
      Foo();
      void add(const int);
      int get();
};
Foo::Foo() : mutex(), values() {}
void Foo::add(const int value)
{
   mutex.lock();
   values.push_back(value);
   mutex.unlock();
}
int Foo::get()
{
   mutex.lock();
   int value;
   if ( values.size() > 0 )
   {
      value = values.back();
      values.pop_back();
   }
   else
   {
      value = 0;
   }
   mutex.unlock();
   return value;
}

Disclaimer: The default value of 0 in get() is intended as it has a special meaning in the rest of the code.

Update: The above code is exactly as I use it, except for the typo push_Back of course.

Upvotes: 2

Views: 3000

Answers (2)

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275790

Here is the "improved" version:

#include <mutex>
#include <vector>
class Foo {
   private:
      std::mutex mutex;
      typedef std::lock_guard<std::mutex> lock;
      std::vector<int> values;
   public:
      Foo();
      void add(int);
      int get();
};
Foo::Foo() : mutex(), values() {}
void Foo::add(int value) {
   lock _(mutex);
   values.push_back(value);
}
int Foo::get() {
   lock _(mutex);
   int value = 0;
   if ( !values.empty() )
   {
      value = values.back();
      values.pop_back();
   }
   return value;
}

with RAII for acquiring the mutex etc.

Upvotes: 1

David Schwartz
David Schwartz

Reputation: 182829

Other than not using RAII to acquire the lock and using size() > 0 instead of !empty(), the code looks fine. This is exactly how a mutex is meant to be used and this is the quintessential example of how and where you need a mutex.

As Andy Prowl pointed out, instances can't be copy constructed or copy assigned.

Upvotes: 2

Related Questions