progschj / ThreadPool

A simple C++11 Thread Pool implementation
zlib License
7.63k stars 2.21k forks source link

Some code seem to be incorrect that while task queue has task but pool don't excute them. #76

Closed JohnGu9 closed 4 years ago

JohnGu9 commented 4 years ago

origin code

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();
    return res;
}

In the 13nd line std::unique_lock<std::mutex> lock(queue_mutex); This code seem to be an invaild lock that lead to task can't run after push back into queue.

I change this a bit. std::lock_guard<std::mutex> lock(queue_mutex); and work properly.

wilx commented 4 years ago

I don't see the problem with unique_lock here. Can you expand on to why you think it is wrong?

JohnGu9 commented 4 years ago

Sorry, I find out it is my mistake that sometimes my task will be blocked by system api :( unique_lock is work fine. Maybe lock_guard is lightweight that toggle my mistake less times somehow.