notify_one in ThreadPool::enqueue - valgrind error about threadpool

[yinchunxiang]

// add new work item to the pool
template<class F, class... Args>
auto ThreadPool::enqueue(F&& f, Args&&... args) 
    -> std::future<typename std::result_of<F(Args...)>::type>
{
    using return_type = typename std::result_of<F(Args...)>::type;

    auto task = std::make_shared< std::packaged_task<return_type()> >(
            std::bind(std::forward<F>(f), std::forward<Args>(args)...)
        );

    std::future<return_type> res = task->get_future();
    {
        std::unique_lock<std::mutex> lock(queue_mutex);

        // don't allow enqueueing after stopping the pool
        if(stop)
            throw std::runtime_error("enqueue on stopped ThreadPool");

        tasks.emplace([task](){ (*task)(); });
    }
**    condition.notify_one(); ///----> Should this line be in the lock scope ?**
    return res;
}

// the destructor joins all threads
inline ThreadPool::~ThreadPool()
{
    {
        std::unique_lock<std::mutex> lock(queue_mutex);
        stop = true;
    }
**    condition.notify_all(); ///----> Should this line be in the lock scope ?**
    for(std::thread &worker: workers)
        worker.join();
}
```cpp

[foobar]

condition.notify_one(); ///----> Should this line be in the lock scope ?

would it cause a deadlock ?

[mnowotnik]

Not deadlock, but the thread could never wake up.

[lazylazypig]

@mgerhardy
excuse me, what's the final solution?

[wilx]

I think that in general it is a good idea to notify inside the locked region because you can miss wakeups otherwise. But this code is not edge triggered but level triggered, I think. In this particular case, I do not think it is possible to miss a wake up.

[ilyapopov]

This SO question is relevant: http://stackoverflow.com/questions/17101922/do-i-have-to-acquire-lock-before-calling-condition-variable-notify-one/

TL;DR: No, you don't need to hold a lock while doing notify, but there can be performance implications.