Trade off between a recursive mutexes V a more complex class?

Question

I've heard that recursive mutexs are evil but I can't think of the best way to make this class work without one.

class Object {
public:
    bool check_stuff() {
        lock l(m);

        return /* some calculation */;
    }
    void do_something() { 
        lock l(m);

        if (check_stuff()) {
            /* ... */
        }
        /* ... */
    }
private:
    mutex m;
}

The function do_something() will deadlock if m isn't recursive but what's the alternative unless I have, say, a two ways of performing the check one of which doesn't lock.

That choice, as the link above suggests, will ultimately makes the class more complex, is there a neater solution which doesn't require a recursive mutex? Or is a recursive mutex sometimes not that evil?

Answer 1

Make check_stuff private, and not lock, and simply call it from do_something with the mutex locked. A common idiom to use to make this safer is to pass a reference to the lock to check_stuff:

 bool check_stuff(const lock&)
 {
    return /* some calculation */;
 }

 void do_something()
 {
    lock l(m);
    if(check_stuff(l)) { // can't forget to lock, because you need to pass it in to call check_stuff
      ...
    }
 }

check_stuff shouldn't lock and return a bool anyway, any value it returns is out of date by the time you look at it, making it pretty useless for consumers without access to the lock.

Answer 2

If you need to expose check_stuff as public (as well as do_something which currently calls it), a simple refactoring will see you through without repeating any calculations:

class Object {
public:
    bool check_stuff() {
        lock l(m);

        return internal_check_stuff(l);
    }
    void do_something() { 
        lock l(m);

        if (internal_check_stuff(l)) {
            /* ... */
        }
        /* ... */
    }
private:
    mutex m;
    bool internal_check_stuff(lock&) {
        return /* some calculation */;
    }
}

(as per @Logan's answer, passing a lock reference to the internal function is just a good idea to avoid forgetfulness from future maintainers;-).

Recursive locks are only needed when you call a method which locks from another one which also does (with the same lock), but a little refactoring (moving the common functionality into private methods that don't lock but take lock references as "reminders";-) removes the need.

No requirement to repeat any functionality -- just make public functions which expose exactly the same (locking) functionality that's also needed (in non-locking ways because the lock is already acquired) from other functions, into "semi-empty shells" that perform locking then call such private functions, which do all the necessary work. The other functions in the class that need such functionality but want to do their own locking clearly can just call the private ones directly.