CODEWITHSUNDEEP
c++mutex
daohu527
daohu527

Reputation: 512

Call a member function with lock?

I design a queue like below, and one question is if the member function have a lock in it, I want to call it in another member function, which have a lock in it too. This will cause the problem of reentrant locks?

class MyQueue {
 public:
  bool Empty() {
   std::lock_guard<std::mutex> lock(mutex_);
   return queue_.size() == 0;
  }
  
  bool Dequeue(std::string& data) {
    std::lock_guard<std::mutex> lock(mutex_);
    if (Empty()) return false;    // reentrant locks error!!!
    data = queue_.front();
    queue_.pop();
    return true;
  }
}

My solution is not to call the member function, what's the best practices to avoid it?

  bool Dequeue(std::string& data) {
    std::lock_guard<std::mutex> lock(mutex_);
    if (queue_.size() == 0) return false;    // don't call member function!!!
    data = queue_.front();
    queue_.pop();
    return true;
  }

Upvotes: 1

Views: 1299

Answers (2)

ivan.ukr
ivan.ukr

Reputation: 3551

In this particular case your last solution is good - call container function. Introducing thing like

private:
    bool IsEmpty() {
        return queue_.empty();
    }

is just unnecessary wrapper around the member function that just bloats up the code.

But if your IsEmpty() function is supposed to do something more, only then it makes sense to have it. But from your code example it doesn't look like this.

Also - one more recommendation about best C++ coding practices - if you want to test container for emptiness or non-emptiness, use container.empty() instead of container.size() == 0.

Upvotes: 1

jignatius
jignatius

Reputation: 6494

If a public member function needs to call another public member function, both of which lock the same mutex, then it's better to split them so that they call a private member function that doesn't lock. The private member function assumes that a lock is already held. This way you avoid having to use a recursive mutex, which often is a sign of a design flaw.

So I would create a private IsEmpty() function that doesn't lock and call that from Dequeue() and any other public member function:

class MyQueue {
 public:
  bool Dequeue(std::string& data) {
    std::lock_guard<std::mutex> lock(mutex_);
    if (IsEmpty()) return false;
    data = queue_.front();
    queue_.pop();
    return true;
  }

  private:
   bool IsEmpty() {
     return queue_.empty();
   }
   ...
};

This will also allow you to work with condition variables in the usual way. With std::condition_variable only one level of lock is released, but if you use a lock on recursive mutex you may still hold the lock and thus deadlock.

You need to ask yourself whether you really need a public function like Empty() in a multithreaded program. If you have different threads adding and popping elements into your queue at any one time, then such a function may not be worth it. However, if your really want a public member function Empty(), it would have to lock the mutex and then call the private IsEmpty() function.

Note that you can just call the member function empty() on a standard container like std::queue instead of doing queue_.size() == 0, in which case you could probably do away with IsEmpty() completely.

Upvotes: 3

Related Questions