0

I have a class that I need to make thread-safe. I'm trying to do this by putting a unique lock at the top of every function in the class. The problem is that as soon as one function calls another function (in this class) the mutexes seem to lock each other, despite being in different functions. How can I stop this from happening?

An example is a class with get() and set() functions that both use a unique_lock at the start of each function. But in set() you want to call get() at some point, but without set()'s mutex locking get()'s mutex. However the mutex in get() should still work if called directly.

Jason
  • 531
  • 6
  • 19
  • 1
    Use recursive mutex ? -> https://stackoverflow.com/questions/27626290/can-unique-lock-be-used-with-a-recursive-mutex – rafix07 Dec 29 '17 at 06:54
  • Thanks, I'll try out using a recursive mutex. – Jason Dec 29 '17 at 07:09
  • 2
    You **accepted the wrong answer**, the one that solves your immediate problem, but not the one that told you that what you're doing is **poor coding and bad practice**. – Walter Oct 30 '18 at 11:27
  • Please read thoroughly before implementing the recursive mutex solution. Neither a recursive mutex nor a unique lock on every class function should be used as a blanket tool for thread-safety. Their use should follow a conscientious analysis of the kind of serialization point that one requires around the shared resource. – Pablo Adames Feb 27 '23 at 18:02

4 Answers4

6

Making a class "thead safe" by adding a mutex to all operations is code smell. Doing so with recursive mutex is worse, because it implies a lack of control and understanding about what was locked and what operations lock.

While it often permits some limited multithreaded access, but leads very often to deadlocks, contention and performance hell down the lane.

Lock based concurrency does not safely compose except in limited cases. You can take two correct lock-based datastructures/algorithms, connect them, and end up with incorrect/unsafe code.

Consider leaving your type single threaded, implementing const methods that can be mutually called without synchronization, then using mixtures of immutable instances and externally synchronized ones.

template<class T>
struct mutex_guarded {
  template<class F>
  auto read( F&& f ) const {
    return access( std::forward<F>(f), *this );
  }
  template<class F>
  auto write( F&& f ) {
    return access( std::forward<F>(f), *this );
  }
  mutex_guarded()=default;
  template<class T0, class...Ts,
    std::enable_if_t<!std::is_same<mutex_guarded, std::decay_t<T0>>, bool> =true
  >
  mutex_guarded(T0&&t0, Ts&&ts):
    t(std::forward<T0>(t0),std::forward<Ts>(ts)...)
  {}
private:
  template<class F, class Self>
  friend auto access(F&& f, Self& self ){
    auto l = self.lock();
    return std::forward<F>(f)( self.t );
  }
  mutable std::mutex m;
  T t;
  auto lock() const { return std::unique_lock<std::mutex>(m); }
};

and similar for shared mutex (it has two lock overloads). access can be made public and vararg woth a bit of work (to handle things like assignment).

Now calling your own methods is no problem. External use looks like:

std::mutex_guarded<std::ostream&> safe_cout(std::cout);
safe_cout.write([&](auto& cout){ cout<<"hello "<<"world\n"; });

you can also write async wrappers (that do tasks in a thread pool and return futures) and the like.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • 2
    Having worked with a code-base where a recursive mutex was the solution to all locking ills, I would second this - you may be trading one set of problem for another. The deadlocks that might occur can be really difficult to debug. You should consider carefully whether you want to encapsulate locking (intrinsic locking) or do it extrinsically. Recursive mutices are a code-smell in their own right almost every time. – marko Dec 29 '17 at 12:02
0

The std::recursive_mutex is what you want. It can be locked more than one time in one thread.

Jks Liu
  • 457
  • 2
  • 12
  • This answer solves a problem that was created artificially by putting a lock on every class function. That is neither necessary nor safe. The original question is ill-posed although extremely useful. No silver bullet I am afraid. Know your requirements and test extensively. Serializing access to common resources shared by many threads is not a trivial exercise that can be approached with a unique recipe. – Pablo Adames Jun 05 '23 at 21:34
0

It could be more clear if you provided the code. The problem arises from the fact that all the locks share the same mutex object. a recursive lock could to some extent solve the problem. but bear in mind that getters do not necessarily have to be locked. I confronted the same problem years ago, There were a couple of threads working together on a proxy object. The final solution was that I had to define more than one mutex. If it is possible, make use of Qt signals or boost signal each of which helps you come up with a better solution for passing data back and forth.

milad hasanvand
  • 1
0

While using std::recursive_mutex will work, it might incur some overhead that can be avoided.

Instead, implement all logic in private methods that do not take the lock but assume the lock is held by the current thread of execution. Then provide the necessary public methods that take the lock and forward the call to the respective private methods. In the implementation of the private methods, you are free to call other private methods without worrying about locking the mutex multiple times.

struct Widget
{
    void foo()
    {
        std::unique_lock<std::mutex> lock{ m };
        foo_impl();
    }

    void bar()
    {
        std::unique_lock<std::mutex> lock{ m };
        bar_impl();
    }

private:
    std::mutex m;

    void foo_impl()
    {
        bar_impl(); // Call other private methods
    }

    void bar_impl()
    {
        /* ... */
    }
};

Note that this is just an alternative approach to (potentially) tackle your problem.

Maarten Bamelis
  • 2,243
  • 19
  • 32