avoiding multiple mutex locks for protecting similar race conditions

Question

Suppose I have some piece of code that gives me a race condition, say

class foo
{
  some_data data;
public:
  void bar(some_type arg)
  {
    // may change data
  }
  // ...
};

Using foo::bar() without protection won't be threadsafe, because another thread may call bar() at the same time. Hence, arguably, a better choice is

class foo
{
  some_data data;
  std::mutex my_mutex;
  void unsafe_bar(some_type arg);
public
  void bar(some_type arg)
  {
     std::lock_guard<std::mutex> lock(my_mutex);
     unsafe_bar(arg);
  }
};

However, suppose there are many similar classes with member functions having this very same problem

class foo1 { public: void bar(some_type arg); /*...*/ };
class foo2 { public: void bar(some_type arg); /*...*/ };
class foo3 { public: void bar(some_type arg); /*...*/ };
class foo4 { public: void bar(some_type arg); /*...*/ };
class foo5 { public: void bar(some_type arg); /*...*/ };

each called within one block of code:

void work(some_type arg, foo1&f1, foo2&f2, foo3&f3, foo4&f4, foo5&f5)
{
  f1.bar(arg);
  f2.bar(arg);
  f3.bar(arg);
  f4.bar(arg);
  f5.bar(arg);
}

Now, each of these calls locks and unlocks another mutex, which seems inefficient. It would be better to use just one mutex.

Q Is there a recommended/best way to do this?

I was thinking about the following design:

class foo
{
  std::mutex my_mutex;
  void unsafe_bar(some_type arg);
public:
  template<typename Mutex>
  void bar(some_type arg, Mutex&m)
  {
    std::lock_guard<Mutex> lock(m);
    unsafe_bar(arg);
  }
  void bar(some_type arg)
  {
    bar(arg,my_mutex);
  }
};

Now the user can either rely on foo::my_mutex (2nd version of foo::bar), or provide a mutex, which can be a std::recursive_mutex or a null_mutex (which satisfies the mutex concept, but does nothing). Q Is this a sensible/useful idea?

EDIT I noticed that my race is actually not dependent on any particular iterator (and its possible invalidation etc). I removed any reference to an iterator.

Answer 1

Is there a recommended/best way to do this?

I think your approach is quite common and a good way to avoid a race condition.

Is this a sensible/useful idea?

I think the second approach is more than questionable. You will put the burden of synchronizing upon your client/user of your class. At least whenever the templated function is used. But if you do this you can just declare your class as not thread safe and the caller must synchronize it in all cases. Which would be at least consistent.

In general you should synchronize on the data, on data members that are accessed concurrently. The number of your mutexes is more about granularity. E.g always lock a complete block or only lock the read/write operation. But this depends on the use case.

Edit: Since you now posted some code.
It still depends on how you want to use your classes.
Are f1 to f5 only used within the
void work(some_iterator const&i, foo1&f1, foo2&f2, foo3&f3, foo4&f4, foo5&f5) method. Then don't use a mutex in them at all. It is easy for the user of the classes to do the synchronization. A mutex in work.
Are the classes used separately from the work method?
Again the answer will be protect each class member that is used with it's own mutex, because for the client it will be much more difficult.

Answer 2

tl;dr Ultimately, how you setup thread synchronization on your objects is up to you and while calling a lock on a mutex from separate objects might not seem efficient, you take that into account in your overall design and figure out 'where' you want your mutex/semaphore's and other synchronization objects to best handle the overhead associated with making your code 'thread safe'. And if you are writing code that will be used externally (like library code), then you'll need to be sure you document that a particular function is in fact 'thread safe' otherwise I (as the user) will take it on myself to protect my own code.

void work(some_iterator const&i, foo1&f1, foo2&f2, foo3&f3, foo4&f4, foo5&f5)
{
    f1.bar(i);
    f2.bar(i);
    f3.bar(i);
    f4.bar(i);
    f5.bar(i);
}

Q Is there a recommended/best way to do this?

Yes, take the mutex out of the foo objects and put it elsewhere;

// defined somewhere
std::mutex _mtx;

void work(some_iterator const&i, foo1&f1, foo2&f2, foo3&f3, foo4&f4, foo5&f5)
{
    _mtx.lock();
    f1.bar(i);
    f2.bar(i);
    f3.bar(i);
    f4.bar(i);
    f5.bar(i);
    _mtx.unlock();
}

Q Is template<typename Mutex> void bar(some_iterator const&i, Mutex&m) a sensible/useful idea?

No, as the example above shows, it's much simpler (and cleaner) to call a 'global' mutex/semphore object; if I wanted to use your templated function doing the same exmaple, I'd have extra typing, extra calls being made, and quite possibly not the effect that I'd want, ex:

// defined somewhere
std::mutex _mtx;

void work(some_iterator const&i, foo1&f1, foo2&f2, foo3&f3, foo4&f4, foo5&f5)
{
    f1<std::mutex>.bar(i, _mtx);
    f2<std::mutex>.bar(i, _mtx);
    f3<std::mutex>.bar(i, _mtx);
    f4<std::mutex>.bar(i, _mtx);
    f5<std::mutex>.bar(i, _mtx);
}

I'd have to know about the mutex's in the work function specifically because my some_iterator might get modified outside of the work function (even though each of your foo objects has a mutex), example:

void work(some_iterator const&i, foo1&f1, foo2&f2, foo3&f3, foo4&f4, foo5&f5)
{
    // i could be modified here
    f1.bar(i); // here if mutex handle != others
    // here
    f2.bar(i); // here if mutex handle != others
    // here
    f3.bar(i); // here if mutex handle != others
    // here
    f4.bar(i); // here if mutex handle != others
    // here
    f5.bar(i); // here if mutex handle != others
    // here
}

Of course, this might be what you want? If the work function is not designed to be atomic in nature, then I (as a user of the function) might put a mutex lock around the work function, example (assuming the above code):

std::mutex _mtx2;

void some_thread_fn1()
{
    _mtx2.lock();
    // some other code
    work(i, f1, f2, f3, f4, f5);
    _mtx2.unlock();
}

void some_thread_fn2()
{
    _mtx2.lock();
    // some other code
    work(i, f1, f2, f3, f4, f5);
    _mtx2.unlock();
}

Who ever uses the work function would need to be (somewhat) aware that a mutex is called in it, otherwise they might doubley protect (defeating your original purpose).

Also note that none of your code would actually lock the same mutex (assuming your foo classes have std::mutex's in them as well), example:

class foo1
{
    some_data data;
    std::mutex my_mutex;
    void unsafe_bar(some_iterator const&i);
public:
    void bar(some_iterator const&i)
    {
        std::lock_guard<std::mutex> lock(my_mutex);
        unsafe_bar(i);
    }
};

class foo2
{
    some_data data;
    std::mutex my_mutex;
    void unsafe_bar(some_iterator const&i);
public:
    void bar(some_iterator const&i)
    {
        std::lock_guard<std::mutex> lock(my_mutex);
        unsafe_bar(i);
    }
};


foo1 f1;
foo2 f2;
some_iterator x;

void thread1()
{
    f1.bar(x);
}

void thread2()
{
    f2.bar(x);
}

The above code would cause a race condition because you are locking 2 separate mutexes (a logic error as you want to lock on the same mutex).

This question seems more about multi-threaded design and best practices for it, as another answer pointed out.

Hope that can help.