Thread-safe vector: Is this implementation thread-safe?

Question

I have a question regarding the term thread-safety. Let me give an example:

#include <mutex>
#include <vector>

/// A thread-safe vector
class ThreadSafeVector {
private:
  std::mutex m;
  std::vector<double> v;

public:
  // add double to vector
  void add(double d) {
    std::lock_guard<std::mutex> lg(m);
    v.emplace_back(d);
  }

  // return length of vector  
  int length() {
    std::lock_guard<std::mutex> lg(m);
    return v.size();
  }   
};

Would you call that class, i.e. all its methods, thread-safe?

EDIT [Sunday, 9 PM CEST]

After getting some good "yes, but"-answers and alternative implementations, I provided my own view in an answer below. Basically, it boils down to the simple question, whether thread-safety of a class only has to make strong atomicity and visibility guarantees for PARALLEL execution of its methods OR whether a class has to make guarantees that stretch beyond its own scope (for example SERIAL execution).

Answer 1

IMHO:

This is both safe and useful:

  void add(double d) {
    std::lock_guard<std::mutex> lg(m);
    v.emplace_back(d);
  }

This is safe but useless:

  // return length of vector  
  int length() {
    std::lock_guard<std::mutex> lg(m);
    return v.size();
  }   

Because by the time you've got your length it may well have changed, so reasoning about it is unlikely to be useful.

How about this?

template<class Func>
decltype(auto) do_safely(Func&& f)
{
  std::lock_guard<std::mutex> lock(m);
  return f(v);
}

called like this:

myv.do_safely([](auto& vec) { 
  // do something with the vector
  return true;  // or anything you like
});

Answer 2

What you have provided is thread-safe. However the problem with this is that you can't add methods that allow access to the elements without loosing thread safety. Also this kind of thread-safety is very inefficient. Sometimes you want to iterate over the whole container and sometimes you want to add many elements one after another.

As an alternative you can put the responsibility for locking on the caller. This is much more efficient.

/// A lockable vector
class LockableVector
{
public:
    using mutex_type = std::shared_timed_mutex;
    using read_lock = std::shared_lock<mutex_type>;
    using write_lock = std::unique_lock<mutex_type>;

    // default ctor
    LockableVector() {}

    // move-ctor
    LockableVector(LockableVector&& lv)
    : LockableVector(std::move(lv), lv.lock_for_writing()) {}

    // move-assign
    LockableVector& operator=(LockableVector&& lv)
    {
        lv.lock_for_writing();
        v = std::move(lv.v);
        return *this;
    }

    read_lock lock_for_reading() { return read_lock(m); }
    write_lock lock_for_writing() { return write_lock(m); }

    // add double to vector
    void add(double d) {
        v.emplace_back(d);
    }

    // return length of vector
    int length() {
        return v.size();
    }

    // iteration
    auto begin() { return v.begin(); }
    auto end() { return v.end(); }

private:
    // hidden, locked move-ctor
    LockableVector(LockableVector&& lv, write_lock)
    : v(std::move(lv.v)) {}

    mutex_type m;
    std::vector<double> v;
};

int main()
{
    LockableVector v;

    // add lots of elements

    { /// create a scope for the lock
        auto lock = v.lock_for_writing();

        for(int i = 0; i < 10; ++i)
            v.add(i);
    }

    // print elements

    { /// create a scope for the lock
        auto lock = v.lock_for_reading();

        std::cout << std::fixed << std::setprecision(3);
        for(auto d: v)
            std::cout << d << '\n';
    }
}

Also by having both read and write locks you increase efficiency because you can have multiple readers at the same time when no thread is currently writing.

Answer 3

While this is thread-safe, it's not efficient. You could easily make it more efficient by using a shared_mutex (either C++14 or Boost, it's not in C++11). This is because if two threads ask for the size, this should not be a problem. However, if a thread asks for the size and another wanted to add an element, then only one of them should be allowed access.

So I would change your code like this:

#include <mutex>
#include <vector>
#include <shared_mutex>

/// A thread-safe vector
class ThreadSafeVector {
private:
  mutable std::shared_timed_mutex m; //notice the mutable
  std::vector<double> v;

public:
  // add double to vector
  void add(double d) {
    std::unique_lock<std::shared_timed_mutex> lg(m); //just shared_mutex doesn't exist in C++14, you're welcome to use boost::shared_mutex, it's the same
    v.emplace_back(d);
  }

  // return length of vector  
  //notice the const, because this function is not supposed to modify your class
  int length() const {
    std::shared_lock<std::shared_timed_mutex> lg(m);
    return v.size();
  }   
};

A few things to keep in mind:

• std::mutex (and all other mutexes) are non-copyable. This means that your class is now non-copyable. To make it copyable, you have to implement the copy-constructor yourself and bypass copying the mutex.

• always make your mutexes mutable in containers. This is because modifying a mutex doesn't mean you're modifying the content of the class, which is compatible with the const I added to the length() method. That const means that this method doesn't modify anything within the class. It's a good practice to use it.

Answer 4

Though your vector may look like thread safe as soon as you start to use it you will see that it is not. For example, I want to add tasks to a vector if it is smaller than 5 (keep it not bigger than 5)

ThreadSafeVector tv;

if( tv.length() < 5 ) tv.add( 10.0 );

would this properly work in multi-thread environment? No. As you would add more logic to your vector, that will become more and more complicated.

Answer 5

Yes, I would. Both public methods are protected by locks, and all the special member functions (copy/move constructor/assignment) are implicitly deleted because std::mutex is neither copyable nor movable.

Answer 6

We have an on-going internal team discussion about the meaning of thread-safety. Slavas comment "Though length() is technically "thread safe" in reality it is not" boils it down to the ambivalent essence. Probably, it is impossible to answer my simple question with "yes" or "no"?

Here, is my view: Thread-safety only requires clean semantics regarding PARALLEL execution of its operations. The class ThreadSafeVector is thread-safe because its functions guarantee the following for parallel execution of its operations:

• atomicity: interleaving threads will not result in an inconsistent state (because of locking)
• visibility: state is propagated to be seen by other threads (because of the memory-barriers induced by mutex-locking)

Calling a class thread-safe does not require that any possible aggregated usage of it has to be thread-safe on its own, i.e. serial execution of methods on the class does not have to be thread-safe. Example:

if (a.length() == 0) a.add(42);

Of course this line is not thread-safe because it's not atomic on its own and the class does not even provide "tools" to do something like this. But just because I can construct a non-thread-safe sequence from thread-safe operations, it does not mean that the thread-safe operations are really not thread-safe.

Answer 7

One of the best and most error free ways of wrapping thread safety around a traditionally thread-unsafe class is to use a monitor:

template<class T>
class monitor
{
public:
    template<typename ...Args>
    monitor(Args&&... args) : m_cl(std::forward<Args>(args)...){}

    struct monitor_helper
    {
        monitor_helper(monitor* mon) : m_mon(mon), m_ul(mon->m_lock) {}
        T* operator->() { return &m_mon->m_cl;}
        monitor* m_mon;
        std::unique_lock<std::mutex> m_ul;
    };

    monitor_helper operator->() { return monitor_helper(this); }
    monitor_helper ManuallyLock() { return monitor_helper(this); }
    T& GetThreadUnsafeAccess() { return m_cl; }

private:
    T           m_cl;
    std::mutex  m_lock;
};

This will let you access all the methods of the wrapped class in a thread safe way:

monitor<std::vector<int>> threadSafeVector {5};

Then use

threadSafeVector->push_back(5);

or any other member function to have the call performed under a lock. See my original answer here for more information.

This will not magically make multiple calls logically threadsafe (as discuessed in other answers) but there's a way with this system to achieve this aswell:

// You can explicitly take a lock then call multiple functions
// without the overhead of a relock each time. The 'lock handle'
// destructor will unlock the lock correctly. This is necessary
// if you want a chain of logically connected operations 
{
    auto lockedHandle = threadSafeVector.ManuallyLock();
    if(!lockedHandle->empty())
    {
        lockedHandle->pop_back();
        lockedHandle->push_back(-3);
    }
}