CODEWITHSUNDEEP
c++concurrency
amplifier
amplifier

Reputation: 1833

Reusing of std::unique_lock instead of creating a new one

One way to use std::unique_lock is

while (!m_exit || !m_queueOfTasks.empty()) 
{
    std::unique_lock<std::mutex> ul(m_mutex);
    std::cout << "Thread " << std::this_thread::get_id() << " is ready" << std::endl;
    m_cond.wait(ul, [this]() {return !m_queueOfTasks.empty(); });
    std::function<void()> work(std::move(m_queueOfTasks.front()));
    m_queueOfTasks.pop_front();
    std::cout << "Thread " << std::this_thread::get_id() << " got a new task" << std::endl;
    ul.unlock();
    work();
}

We could reuse the unique_lock instead of creating it every iteration.

Since its constructor

Constructs a unique_lock with m as the associated mutex. Additionally locks the associated mutex by calling m.lock().

It means that we need to create it unlocked and lock it manually. Is it the correct code or I missed anything?

std::unique_lock<std::mutex> ul(m_mutex, std::defer_lock);
while (!m_exit || !m_queueOfTasks.empty()) 
{
    ul.lock();
    std::cout << "Thread " << std::this_thread::get_id() << " is ready" << std::endl;
    m_cond.wait(ul, [this]() {return !m_queueOfTasks.empty(); });
    std::function<void()> work(std::move(m_queueOfTasks.front()));
    m_queueOfTasks.pop_front();
    std::cout << "Thread " << std::this_thread::get_id() << " got a new task" << std::endl;
    ul.unlock();
    work();
}

The question is rather technical than an attempt of optimization.

Upvotes: 0

Views: 137

Answers (1)

Nelfeal
Nelfeal

Reputation: 13269

There is one thing that can break complex code if you use this pattern (but won't break your snippets): an exception being thrown.

Consider this:

// static variable, or more realistically a class member
static std::unique_lock<std::mutex> ul(m_mutex, std::defer_lock);
while (!m_exit || !m_queueOfTasks.empty()) 
{
    ul.lock();
    std::cout << "Thread " << std::this_thread::get_id() << " is ready" << std::endl;
    m_cond.wait(ul, [this]() {return !m_queueOfTasks.empty(); });
    // exception thrown!
    std::function<void()> work(std::move(m_queueOfTasks.front()));
    m_queueOfTasks.pop_front();
    std::cout << "Thread " << std::this_thread::get_id() << " got a new task" << std::endl;
    ul.unlock();
    work();
}
// m_mutex might still be locked

If the lock is not just a local variable and anything throws an exception between lock() and unlock(), then the mutex stays locked.

It can also happen if the lock is a local variable but you also have a try-catch block:

std::unique_lock<std::mutex> ul(m_mutex, std::defer_lock);
while (!m_exit || !m_queueOfTasks.empty()) 
{
    try {
        ul.lock();
        std::cout << "Thread " << std::this_thread::get_id() << " is ready" << std::endl;
        m_cond.wait(ul, [this]() {return !m_queueOfTasks.empty(); });
        // exception thrown!
        std::function<void()> work(std::move(m_queueOfTasks.front()));
        m_queueOfTasks.pop_front();
        std::cout << "Thread " << std::this_thread::get_id() << " got a new task" << std::endl;
        ul.unlock();
    } catch(...) {
        std::cout << "Exception thrown";
    }
    // m_mutex might still be locked
    work();
}

These things can quickly happen when working on complicated code, and is one of the reasons using scopes (and RAII) is recommended. This potential bug cannot happen when using the scope:

while (!m_exit || !m_queueOfTasks.empty()) 
{
    {
        std::unique_lock<std::mutex> ul(m_mutex, std::defer_lock);
        std::cout << "Thread " << std::this_thread::get_id() << " is ready" << std::endl;
        m_cond.wait(ul, [this]() {return !m_queueOfTasks.empty(); });
        // exception thrown!
        std::function<void()> work(std::move(m_queueOfTasks.front()));
        m_queueOfTasks.pop_front();
        std::cout << "Thread " << std::this_thread::get_id() << " got a new task" << std::endl;
    } // lock destructor always called
    // m_mutex is unlocked no matter what
    work();
}

Upvotes: 1

Related Questions