CODEWITHSUNDEEP
c++multithreadingthread-safetylockingthreadpool
Johannes Stricker
Johannes Stricker

Reputation: 1781

Is this threadpool usage safe?

I'm posting several jobs to a threadpool and then waiting for it to finish. I'm wondering if I've missed something here, since occasionally my worker threads seem to freeze.

My main thread start the workers like this:

numJobsPosted = 0;
for(auto entry : list)
{
    numJobsPosted++;
    threadPool->post(std::bind(&Controller::workerFunc, this, entry));
}

std::unique_lock<std::mutex> lock(m_workerLock);
while(numJobsPosted > 0)
{
    m_workerCondition.wait(lock);
}

Now my workerFunc looks something like this:

void Controller::workerFunc(Entry entry)
{
    // do some work with entry

    // notify finished
    numJobsPosted--;
    if(numJobsPosted <= 0)
    {
        // does the look need to be around the numJobsPosted-- ?
        std::unique_lock<std::mutex> locker(m_workerLock);
        m_workerCondition.notify_one();
    }
}

Is the above code safe, or do I need to put the lock around the decrement operator?

Upvotes: 3

Views: 69

Answers (1)

Martin J.
Martin J.

Reputation: 5118

This may depend on details of your thread pool's inner logic or setup (e.g. if you have a single thread, so jobs are actually run sequentially), but assuming that numJobsPosted is an int or similar built-in type, your code isn't thread-safe.
This line in workerFunc:

numJobsPosted--;

could very well be the subject of a race condition if it gets executed by several jobs concurrently.

Also, I'm not sure what your threadpool's post function does precisely, but if it dispatches the worker function to a thread right away and some of the worker functions can return immediately, you have another possible race condition between this line in your main thread code:

numJobsPosted++;

and this line in workerFunc:

numJobsPosted--;

To make it safe, you can for instance make numJobsPosted atomic, e.g. declare it like this (in C++11):

#include <atomic>
std::atomic_int numJobsPosted;

Making your workerFunc something like this:

void Controller::workerFunc(Entry entry)
{
    // do some work with entry

    // notify finished
    {
        std::unique_lock<std::mutex> locker(m_workerLock);
        numJobsPosted--;
        if(numJobsPosted <= 0)
        {
            m_workerCondition.notify_one();
        }
    }
}

may solve the first race condition case, but not the second.

(Also, I don't really understand the logic around the manipulation and testing you're doing on numJobsPosted, but I think that's beside the point of your question)

Upvotes: 2

Related Questions