How to implement thread-safe container with natural looking syntax?

Question

Preface

Below code results in undefined behaviour, if used as is:

vector<int> vi;
...
vi.push_back(1);  // thread-1
...
vi.pop(); // thread-2

Traditional approach is to fix it with std::mutex:

std::lock_guard<std::mutex> lock(some_mutex_specifically_for_vi);
vi.push_back(1);

However, as the code grows, such things start looking cumbersome, as everytime there will be a lock before a method. Moreover, for every object, we may have to maintain a mutex.

Objective

Without compromising in the syntax of accessing an object and declaring an explicit mutex, I would like to create a template such that, it does all the boilerplate work. e.g.

Concurrent<vector<int>> vi;  // specific `vi` mutex is auto declared in this wrapper
...
vi.push_back(1); // thread-1: locks `vi` only until `push_back()` is performed
...
vi.pop ()  // thread-2: locks `vi` only until `pop()` is performed

In current C++, it's impossible to achieve this. However, I have attempted a code where if just change vi. to vi->, then the things work as expected in above code comments.

Code

// The `Class` member is accessed via `->` instead of `.` operator
// For `const` object, it's assumed only for read purpose; hence no mutex lock
template<class Class,
         class Mutex = std::mutex>
class Concurrent : private Class
{
  public: using Class::Class;

  private: class Safe
           {
             public: Safe (Concurrent* const this_,
                           Mutex& rMutex) :
                     m_This(this_),
                     m_rMutex(rMutex)
                     { m_rMutex.lock(); }
             public: ~Safe () { m_rMutex.unlock(); }

             public: Class* operator-> () { return m_This; }
             public: const Class* operator-> () const { return m_This; }
             public: Class& operator* () { return *m_This; }
             public: const Class& operator* () const { return *m_This; }

             private: Concurrent* const m_This;
             private: Mutex& m_rMutex;
           };

  public: Safe ScopeLocked () { return Safe(this, m_Mutex); }
  public: const Class* Unsafe () const { return this; }

  public: Safe operator-> () { return ScopeLocked(); }
  public: const Class* operator-> () const { return this; }
  public: const Class& operator* () const { return *this; }

  private: Mutex m_Mutex;
};

Demo

Questions

• Is using the temporary object to call a function with overloaded operator->() leads to undefined behavior in C++?
• Does this small utility class serve the purpose of thread-safety for an encapsulated object in all the cases?

---

Clarifications

For inter-dependent statements, one needs a longer locking. Hence, there is a method introduced: ScopeLocked(). This is an equivalent of the std::lock_guard(). However the mutex for a given object is maintained internally, so it's still better syntactically.
e.g. instead of below flawed design (as suggested in an answer):

if(vi->size() > 0)
  i = vi->front(); // Bad: `vi` can change after `size()` & before `front()`

One should rely on below design:

auto viLocked = vi.ScopeLocked();
if(viLocked->size() > 0)
  i = viLocked->front();  // OK; `vi` is locked till the scope of `viLocked`

In other words, for the inter-dependent statements, one should be using the ScopeLocked().

Answer 1

Don't do this.

It's almost impossible to make a thread safe collection class in which every method takes a lock.

Consider the following instance of your proposed Concurrent class.

Concurrent<vector<int>> vi;

A developer might come along and do this:

 int result = 0;
 if (vi.size() > 0)
 {
     result = vi.at(0);
 }

And another thread might make this change in between the first threads call to size() and at(0).

vi.clear();

So now, the synchronized order of operations is:

vi.size()  // returns 1
vi.clear() // sets the vector's size back to zero
vi.at(0)   // throws exception since size is zero

So even though you have a thread safe vector class, two competing threads can result in an exception being thrown in unexpected places.

That's just the simplest example. There are other ways in which multiple threads attempting to read/write/iterate at the same time could inadvertently break your guarantee of thread safety.

You mentioned that the whole thing is motivated by this pattern being cumbersome:

vi_mutex.lock();
vi.push_back(1);
vi_mutex.unlock();

In fact, there are helper classes that will make this cleaner, namely lock_guard that will take a mutex to lock in its constructor and unlock on the destructor

{
    lock_guard<mutex> lck(vi_mutex);
    vi.push_back(1);
}

Then other code in practice becomes thread safe ala:

{
     lock_guard<mutex> lck(vi_mutex);
     result = 0;
     if (vi.size() > 0)
     {
         result = vi.at(0);
     }
}

Update:

I wrote a sample program, using your Concurrent class to demonstrate the race condition that leads to a problem. Here's the code:

Concurrent<list<int>> g_list;

void thread1()
{
    while (true)
    {
        if (g_list->size() > 0)
        {
            int value = g_list->front();
            cout << value << endl;
        }
    }

}

void thread2()
{
    int i = 0;
    while (true)
    {
        if (i % 2)
        {
            g_list->push_back(i);
        }
        else
        {
            g_list->clear();
        }
        i++;
    }
}

int main()
{

    std::thread t1(thread1);
    std::thread t2(thread2);

    t1.join(); // run forever

    return 0;
}

In a non-optimized build, the program above crashes in a matter of seconds. (Retail is a bit harder, but the bug is still there).