Proper Granularity for Mutex Lock

Question

I am working on a program that prints regex matches from several threads searching the filesystem and each thread adds its id to a queue and notifies the main thread that there are some threads to be removed from the thread pool which is a std::vector.

At this point, I am unlocking and locking the mutex whenever I perform an operation on the resource to be protected. It feels like I'm thinking of this all wrong and that I'm being way too granular. Would it be better to lock the mutex before entering the while loop, unlock it after popping the queue, and then locking it again before !finished_threads.empty() is called on the next iteration?

void RegexSearch::Run()
{
    running = true;
    search_pool.push_back(std::thread([&] { StartPrint(); }));

    /*
    Get directories and create another thread for each subdirectory while there are
    less threads than maximum in the pool.
    */

    fs::path searchDir(directory);

    for (const auto& dir : searchDir)
    {
        if (search_pool.size() >= MAX_THREADS)
        {
            std::unique_lock<std::mutex> lck(mu, std::defer_lock);
            // Wait to be notified that thread is finished execution and another can start.
            max_thread_cond.wait(lck);

            lck.lock();
            while (!finished_threads.empty())
            {
                lck.unlock();

                lck.lock();
                std::thread::id remove_thread = finished_threads.front();
                lck.unlock();

                lck.lock();
                finished_threads.pop();
                lck.unlock();

                std::remove_if(search_pool.begin(), search_pool.end(), [&remove_thread](const std::thread::id &t) { return remove_thread == t; });
                lck.lock();
            }
        }
    }
}

Bo Persson Bo Persson · Accepted Answer · 2016-12-20T07:47:18

You certainly have to keep the lock from the test for !empty() until you have removed the item from the queue. Otherwise several threads might try to remove the same item - perhaps the last one.

I also see that the push_back at the start of the function is unprotected. If there might be several threads accessing the search_pool, it must be guarded as well.

Proper Granularity for Mutex Lock

1 Answers