Safely Destroying a Thread Pool

Question

Consider the following implementation of a trivial thread pool written in C++14.

Observe that each thread is sleeping until it's been notified to awaken -- or some spurious wake up call -- and the following predicate evaluates to true:

std::unique_lock<mutex> lock(this->instance_mutex_);

this->cond_handle_task_.wait(lock, [this] {
  return (this->destroy_ || !this->tasks_.empty());
});

Furthermore, observe that a ThreadPool object uses the data member destroy_ to determine if its being destroyed -- the destructor has been called. Toggling this data member to true will notify each worker thread that it's time to finish its current task and any of the other queued tasks then synchronize with the thread that's destroying this object; in addition to prohibiting the enqueue member function.

For your convenience, the implementation of the destructor is below:

ThreadPool::~ThreadPool() {
  {
    std::lock_guard<mutex> lock(this->instance_mutex_); // this line.

    this->destroy_ = true;
  }

  this->cond_handle_task_.notify_all();

  for (auto &worker : this->workers_) {
    worker.join();
  }
}

Q: I do not understand why it's necessary to lock the object's mutex while toggling destroy_ to true in the destructor. Furthermore, is it only necessary for setting its value or is it also necessary for accessing its value?

BQ: Can this thread pool implementation be improved or optimized while maintaining it's original purpose; a thread pool that can pool N amount of threads and distribute tasks to them to be executed concurrently?

This thread pool implementation is forked from Jakob Progsch's C++11 thread pool repository with a thorough code step through to understand the purpose behind its implementation and some subjective style changes.

I am introducing myself to concurrent programming and there is still much to learn -- I am a novice concurrent programmer as it stands right now. If my questions are not worded correctly then please make the appropriate correction(s) in your provided answer. Moreover, if the answer can be geared towards a client who is being introduced to concurrent programming for the first time then that would be best -- for myself and any other novices as well.

The destruction flag is part of the *predicate*. Rule #1of condition-var/mutex/predicate protocol: Never change, *nor even check*, the predicate data unless under the protection of the mutex guarding it. Remember, the mutex guards the predicate data; not the condition variable. If the destruction flag could be modified atomically, it would be exempt. As shown, it isn't. The code shown is correct, and though it may appear overkill, is the correct pattern to learn. — WhozCraig, Mar 21 '15 at 02:36
I think the main confusion here is that a mutex is often an unnecessarily heavy operation for what is really a synchronizing memory-fence or atomic read/write operation, both of which are available as part of the C++11 standard... IMHO using a mutex was the "safe" way of preventing benign data races before atomics, but it's not necessarily the most optimal way now... — Jason, Mar 21 '15 at 02:46
I recommend Anthony Williams' book *C++ Concurrency In Action* to anyone learning concurrent programming in C++, it covers C++11 concurrency, including the thread and atomics library, the C++ memory model, etc. There is code for a simple threadpool in section 9.1. In it the equivalent variable is *std::atomic_bool done* and the destructor simply sets *done=true*, letting the compiler decide if anything other than a store to memory is required (it isn't on x86 because of its Total Store Order memory model). see http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list — amdn, Mar 22 '15 at 20:51

score 1 · Accepted Answer · edited May 23 '17 at 11:50

If the owning thread of the ThreadPool object is the only thread that atomically writes to the destroy_ variable, and the worker threads only atomically read from the destroy_ variable, then no, a mutex is not needed to protect the destroy_ variable in the ThreadPool destructor. Typically a mutex is necessary when an atomic set of operations must take place that can't be accomplished through a single atomic instruction on a platform, (i.e., operations beyond an atomic swap, etc.). That being said, the author of the thread pool may be trying to force some type of acquire semantics on the destroy_ variable without restoring to atomic operations (i.e. a memory fence operation), and/or the setting of the flag itself is not considered an atomic operation (platform dependent)... Some other options include declaring the variable as volatile to prevent it from being cached, etc. You can see this thread for more info.

Without some sort of synchronization operation in place, the worst case scenario could end up with a worker that won't complete due to the destroy_ variable being cached on a thread. On platforms with weaker memory ordering models, that's always a possibility if you allowed a benign memory race condition to exist ...

score 1 · Answer 2 · answered Mar 21 '15 at 15:12

C++ defines a data race as multiple threads potentially accessing an object simultaneously with at least one of those accesses being a write. Programs with data races have undefined behavior. If you were to write to destroy in your destructor without holding the mutex, your program would have undefined behavior and we cannot predict what would happen.

If you were to read destroy elsewhere without holding the mutex, that read could potentially happen while the destructor is writing to it which is also a data race.

Safely Destroying a Thread Pool

2 Answers2

Linked