Race Condition in ThreadPool Destructor

[thejacobpollack]

There's a race condition for the shared resource stop in the ThreadPool destructor. Change the definition of lock to be as follows:

std::lock_guard<std::mutex> lock(this->queue_mutex);

[progschj]

Doesn't unique_lock achieve the same thing as lock_guard here? Admittedly unique_lock is maybe a little overkill in this case. But it should produce the same result.

Also even if stop was unprotected this wouldn't be an actual race condition since only one thread ever writes to stop. The lock is mostly there to force the required memory barrier on some architectures iirc.

[thejacobpollack]

On construction a unique_lock does not lock or unlock a mutex, rather, it only acquires the mutex and becomes responsible for those operations.

In the context of your problem, a lock_guard will acquire the mutex object and immediately lock it. It will then be unlocked when lock goes out of scope.

Edit: I suppose that's true as well -- even with it being unprotected there is only one thread that's going to write to it. The best solution may be to just remove the extra block scope in the destructor?

[progschj]

Aren't we in case 3 here? (http://en.cppreference.com/w/cpp/thread/unique_lock/unique_lock) For the unique_lock to not lock there would have to be an additional defer_lock argument to the construction the way I read that.

[thejacobpollack]

You're absolutely right. I apologize for both of the issues I posted -- I am learning these topics as well and your repository is a great resource.

Thank you for the clarifications on both issues.

[progschj]

No problem. When it comes to this stuff it's always good to have the maximum amount of eyes on the code and resolve any doubts :).