Extraneous call to unlock on scoped_lock

Question

In the following blogpost:

http://www.justsoftwaresolutions.co.uk/threading/implementing-a-thread-safe-queue-using-condition-variables.html

There is a 'push' method defined as follows:

void push(Data const& data)
{
   boost::mutex::scoped_lock lock(the_mutex);
   the_queue.push(data);
   lock.unlock();
   the_condition_variable.notify_one();
}

My questions are:

1. why is there an explicit 'lock.unlock()' being called upon the scoped_lock variable?

2. What is its purpose?

3. Can it be safely removed, resulting in the 'notify_one' method call be within the scope of the scoped_mutex?

Answer 1

The unlock is not necessary. It might reduce the time that the mutex is locked for slightly, however.

Keeping it or removing it will not affect thread safety or result in deadlocks.

EDIT: As the article mentions, however, leaving the unlock in there can result in less contention on the mutex. You might as well leave it in. Alternatively, use scope around the mutex, which I personally find highlights the scope of the mutex better if glancing at the code.

Answer 2

The explicit lock is so the that waiting thread isn't woken up by the notification only to have to block on the mutex. This is explained earlier in the article that you linked to.

The reason to still use a scoped_lock is to ensure that the mutex is properly unlocked in case the push fails and throws an exception.

Answer 3

Points 1 and 2 can be answered by the principle that you should hold your locks for the least time possible. As soon as the data has been pushed onto the queue, you no longer need to lock it.

You can remove the unlock as you've suggested, at the expense of keeping the lock in place for longer.

Alternatively, the unlock itself can be removed by adding a new scope, like so:

void push(Data const& data)
{
   { // new scope
      boost::mutex::scoped_lock lock(the_mutex);
      the_queue.push(data);
   } // scope ends, lock gets destroyed
   the_condition_variable.notify_one();
}