61

I have seen some people hate on recursive_mutex:

http://www.zaval.org/resources/library/butenhof1.html

But when thinking about how to implement a class that is thread safe (mutex protected), it seems to me excruciatingly hard to prove that every method that should be mutex protected is mutex protected and that mutex is locked at most once.

So for object oriented design, should std::recursive_mutex be default and std::mutex considered as an performance optimization in general case unless it is used only in one place (to protect only one resource)?

To make things clear, I'm talking about one private nonstatic mutex. So each class instance has only one mutex.

At the beginning of each public method:

{
    std::scoped_lock<std::recursive_mutex> sl;
14
  • 9
    If it's excruciating to design your code properly then avoid mutexes altogether and use another form of synchronisation such as message queues. Using recursive_mutex not because you need it but just because you can't be bothered to get the locking correct is a hack. A dirty hack. Commented Jan 24, 2013 at 14:53
  • 3
    @JonathanWakely to quote STL: "I disagree in the strongest possible terms".. example I think GC is a tradeoff because you trade perf for dont caring(lets ignore that you want RAII, lets think of hyphotetical C with GC vs C). Point here is that I want my member functions to be called one by one... and I dont care if one of them calls other... I want to free my mind to focus on more important stuff/be more productive. :) Commented Jan 24, 2013 at 15:00
  • 3
    Fine, you're free to do that. You asked "for OO design should std::recursive_mutex be default" and the answer is no. In the general case you should design your locking carefully or not do locking at all. If you can't be bothered, fine. That doesn't make it the correct default for the general case. But then you've already decided to discount the wisdom of Dave Butenhof and ask if you know better, so I'm not expecting you to like my advice. Commented Jan 24, 2013 at 15:22
  • 2
    I read it before commenting. It's a hack. Commented Jan 24, 2013 at 17:45
  • 7
    It makes it impossible to release the mutex (e.g. to use it to wait on a condition variable) temporarily because you have no idea if releasing the current scope's lock will completely release the mutex. It makes it difficult to do anything with two objects at once, e.g. copy assignment, that requires locking both. It can't work in the general case, only a very specific case of an entirely private mutex used in very simple ways. Commented Jan 24, 2013 at 17:57

4 Answers 4

109
+100

Most of the time, if you think you need a recursive mutex then your design is wrong, so it definitely should not be the default.

For a class with a single mutex protecting the data members, then the mutex should be locked in all the public member functions, and all the private member functions should assume the mutex is already locked.

If a public member function needs to call another public member function, then split the second one in two: a private implementation function that does the work, and a public member function that just locks the mutex and calls the private one. The first member function can then also call the implementation function without having to worry about recursive locking.

e.g.

class X {
    std::mutex m;
    int data;
    int const max=50;

    void increment_data() {
        if (data >= max)
            throw std::runtime_error("too big");
        ++data;
    }
public:
    X():data(0){}
    int fetch_count() {
        std::lock_guard<std::mutex> guard(m);
        return data;
    }
    void increase_count() {
        std::lock_guard<std::mutex> guard(m);
        increment_data();
    } 
    int increase_count_and_return() {
        std::lock_guard<std::mutex> guard(m);
        increment_data();
        return data;
    } 
};

This is of course a trivial contrived example, but the increment_data function is shared between two public member functions, each of which locks the mutex. In single-threaded code, it could be inlined into increase_count, and increase_count_and_return could call that, but we can't do that in multithreaded code.

This is just an application of good design principles: the public member functions take responsibility for locking the mutex, and delegate the responsibility for doing the work to the private member function.

This has the benefit that the public member functions only have to deal with being called when the class is in a consistent state: the mutex is unlocked, and once it is locked then all invariants hold. If you call public member functions from each other then they have to handle the case that the mutex is already locked, and that the invariants don't necessarily hold.

It also means that things like condition variable waits will work: if you pass a lock on a recursive mutex to a condition variable then (a) you need to use std::condition_variable_any because std::condition_variable won't work, and (b) only one level of lock is released, so you may still hold the lock, and thus deadlock because the thread that would trigger the predicate and do the notify cannot acquire the lock.

I struggle to think of a scenario where a recursive mutex is required.

11
  • 3
    @NoSenseEtAl: in this example (which remember is "trivial" and "contrived"), the fetch_count function isn't marked const, so there's no benefit in marking the mutex mutable. If it was then there would be. Commented Jan 30, 2013 at 12:22
  • 3
    As @SteveJessop said, you only need mutable on your mutex if it can be used from a const member function. Also: the idea here is to "lock at the beginning of every public method" --- the distinction is that you don't call a public method from another one. Commented Jan 30, 2013 at 14:16
  • 3
    One option is to extract a completely unsynchronized implementation class, so the class with the mutex has just 2 members - a mutex and the implementation class - and only public member functions, which lock the mutex and forward to the implementation class. Commented Jan 31, 2013 at 8:17
  • 7
    The other option is just to say "don't call a public member from another in a class with a mutex" and rely on people sticking to the rule. You'll soon get deadlock in testing if you don't! Commented Jan 31, 2013 at 8:19
  • 2
    "I struggle to think of a scenario where a recursive mutex is required". What about callbacks? Imaging a class that holds pointers to objects within a vector that needs to invoke a callback method on each of them. If that callback method invokes a public method of the original class that also uses the same lock (e.g. pop_back() on the vector) then here is your dead lock.
    – Vassilis
    Commented Sep 21, 2021 at 11:34
28

should std::recursive_mutex be default and std::mutex considered as an performance optimization?

Not really, no. The advantage of using non-recursive locks is not just a performance optimization, it means that your code is self-checking that leaf-level atomic operations really are leaf-level, they aren't calling something else that uses the lock.

There's a reasonably common situation where you have:

  • a function that implements some operation that needs to be serialized, so it takes the mutex and does it.
  • another function that implements a larger serialized operation, and wants to call the first function to do one step of it, while it is holding the lock for the larger operation.

For the sake of a concrete example, perhaps the first function atomically removes a node from a list, while the second function atomically removes two nodes from a list (and you never want another thread to see the list with only one of the two nodes taken out).

You don't need recursive mutexes for this. For example you could refactor the first function as a public function that takes the lock and calls a private function that does the operation "unsafely". The second function can then call the same private function.

However, sometimes it's convenient to use a recursive mutex instead. There's still an issue with this design: remove_two_nodes calls remove_one_node at a point where a class invariant doesn't hold (the second time it calls it, the list is in precisely the state we don't want to expose). But assuming we know that remove_one_node doesn't rely on that invariant this isn't a killer fault in the design, it's just that we've made our rules a little more complex than the ideal "all class invariants always hold whenever any public function is entered".

So, the trick is occasionally useful and I don't hate recursive mutexes to quite the extent that article does. I don't have the historical knowledge to argue that the reason for their inclusion in Posix is different from what the article says, "to demonstrate mutex attributes and thread extensons". I certainly don't consider them the default, though.

I think it's safe to say that if in your design you're uncertain whether you need a recursive lock or not, then your design is incomplete. You will later regret the fact that you're writing code and you don't know something so fundamentally important as whether the lock is allowed to be already held or not. So don't put in a recursive lock "just in case".

If you know that you need one, use one. If you know that you don't need one, then using a non-recursive lock isn't just an optimization, it's helping to enforce a constraint of the design. It's more useful for the second lock to fail, than for it to succeed and conceal the fact that you've accidentally done something that your design says should never happen. But if you follow your design, and never double-lock the mutex, then you'll never find out whether it's recursive or not, and so a recursive mutex isn't directly harmful.

This analogy might fail, but here's another way to look at it. Imagine you had a choice between two kinds of pointer: one that aborts the program with a stacktrace when you dereference a null pointer, and another one that returns 0 (or to extend it to more types: behaves as if the pointer refers to a value-initialized object). A non-recursive mutex is a bit like the one that aborts, and a recursive mutex is a bit like the one that returns 0. They both potentially have their uses -- people sometimes go to some lengths to implement a "quiet not-a-value" value. But in the case where your code is designed to never dereference a null pointer, you don't want to use by default the version that silently allows that to happen.

11
  • 3
    nice answer but I disagree with "something so fundamentally important as whether the lock is allowed to be already held or not. " tbh almost always I dont care if the lock is already held or not. I just want thread safety. Commented Jan 24, 2013 at 10:53
  • @NoSenseEtAl: so how do you avoid locking inversion, if you go around calling things willy-nilly with any old lock held? I suspect that actually you do care a bit ;-) Of course if there's only one lock that could be held then locking inversion isn't a risk: inversion is just one example of something that can go wrong if you don't design and document for what locks are allowed to be held when. Another example is if the inner function uses the mutex to wait on a condvar (releasing the lock) while the outer function really doesn't want the lock released. Commented Jan 24, 2013 at 10:54
  • @ Im talking here about class lvl mtx. Only one, so I just call it at the beginning of every public function. BTW sorry for not replying with @ SO autocomplete feature is broken for me ATM Commented Jan 24, 2013 at 10:58
  • 2
    @NoSenseEtAl: A global lock per class, held by every function called on any instance of that class? Not scalable but yes, I suppose you would want it to be recursive. But what happens if you have two classes with such a lock, and there exists a function of class A that calls some function of class B, and in another thread another function of class B calls some function of class A? Deadlock. FYI, @ replies aren't necessary when addressing the author of the answer, and I believe SO silently removes them in an example of the "So Smug You Want To Slap It" UI design pattern. Commented Jan 24, 2013 at 11:00
  • 2
    "from OO point of view you cant do anything about stuff outside your class" You can do something about it, you can document the pre-conditions for your functions. So you would need to analyze and/or design your code in sufficient depth to identify when a caller needs to acquire both locks before calling your function. Once you understand the code to that depth, you can (if you choose) then design the recursive locks out of it, which is why non-recursive locks are a design decision and not (merely) a performance optimization. Commented Jan 24, 2013 at 11:09
11

I'm not going to directly weigh in on the mutex versus recursive_mutex debate, but I thought it would be good to share a scenario where recursive_mutex'es are absolutely critical to the design.

When working with Boost::asio, Boost::coroutine (and probably things like NT Fibers although I'm less familiar with them), it is absolutely essential that your mutexes be recursive even without the design problem of re-entrancy.

The reason is because the coroutine based approach by its very design will suspend execution inside a routine and then subsequently resume it. This means that two top level methods of a class might "be being called at the same time on the same thread" without any sub calls being made.

0

I recently implemented something like this to do recursion w/o using recursive_mutex:

#include <mutex>
#include <iostream>
#include <type_traits>

struct FakeLock{ FakeLock(std::mutex&){}}; // optimizes to nothing
using RealLock = std::lock_guard<std::mutex>;

class MyClass {
    public:
        template<bool LockMe = true>
        int factorial (int n) {
            // lock is only asserted when LockMe is true
            using Guard = std::conditional_t<LockMe, RealLock, FakeLock>;
            Guard lock(mutex_);
            int ans = 1;
            if (n > 0) {
                // make recursive call w/o asserting the lock
                ans =  n * factorial<false>(n-1);
            }
            return ans;
        }
    private:
        std::mutex mutex_;
};

int main () {
    MyClass wibble;
    int ten = 10;
    int big_number = wibble.factorial(ten);
    std::cout <<  ten << "! = " << big_number << std::endl;
}

output:

10! = 3628800

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.