Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

possible deadlock during ~ThreadPool #115

Open
ShunlongHu opened this issue May 24, 2024 · 6 comments
Open

possible deadlock during ~ThreadPool #115

ShunlongHu opened this issue May 24, 2024 · 6 comments

Comments

@ShunlongHu
Copy link

condition.notify_all(); should be enclosed in the brack of unique_lock.
This is to ensure that all workers are either waiting or executing while notify_all command is dispatched.

@ShunlongHu
Copy link
Author

image

@ShunlongHu ShunlongHu changed the title possible deadlock during Stop possible deadlock during Destruction May 24, 2024
@ShunlongHu ShunlongHu changed the title possible deadlock during Destruction possible deadlock during ~ThreadPool May 24, 2024
@faywong
Copy link

faywong commented Jun 18, 2024

So as the enqueue() method:

     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)(); });
    } // here
    condition.notify_one();

@XiaomingQu0913
Copy link

I don't see this causing a deadlock

@ShunlongHu
Copy link
Author

I don't see this causing a deadlock

All workers should not be within critical section when cv.notify_all is dispatched. Otherwise, the workers may miss the notify_all.

@XiaomingQu0913
Copy link

Although not notified, this->tasks is not empty, so it will not sleep the next time it enters thecritical section.

@Toby-Shi-cloud
Copy link

this is even what cppreference suggested: https://en.cppreference.com/w/cpp/thread/condition_variable
I quote:

manual unlocking is done before notifying, to avoid waking up the waiting thread only to block again (see notify_one for details)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants