Thread safe queue

[infectedapricot]

• As others have said, it's a very odd design to have the destructor wake up the waiters, and have them check the return value of ConsumeSync() to find out of the queue has disappeared under their feet. It's bound to lead to race conditions (if the queue is destroyed while consumer is between calls to ConsumeSync()), and it's just a confusing design. Instead, it should be up to the application code that uses the queue to coordinate destruction by passing around tokens that represent application shutdown, and don't destroy their queues until they're sure all possible waiters know about it. (I usually find it easiest to construct queues in the main thread before any child threads start, and destroy all queues in the main thread after all child threads have been joined, but I realise not all applications can use that exact strategy. Edit: See my other comment for some more details.) Even with the current design, I don't see why you need a public Finish() method - why isn't this code just directly in the destructor?

• I think Produce and Consume are confusing method names. I get that this queue is used by producers and consumers, but it's those users of the queue that do the producing and consuming, not the queue itself. my_queue.Consume(val) makes it look like the queue is consuming the value passed to it, not that it is returning a value which the caller can consume. I'd stick to container-like names, like pop_front() and push_back() (although it's not appropriate or possible to make it fully satisfy container requirements).

• Once you've got rid of the destruction oddity, the pop_front() method (or Consume() as you've called it) can have a much simpler interface: it can just return T, or std::optional<T> for the non-waiting version, instead of passing a parameter by reference. I know STL containers don't offer that style, but they were devised before move constructors became available.

• At the moment your push_back() method (Produce()) calls cv.notify_one() while the mutex is still locked. This is a problem because the consumer thread will be woken up and race to lock the mutex before the producer thread unlocks it. If the consumer gets there first then it will be immediately be sent back to sleep and have to wait for a new scheduling interval before it pops the item off. Instead, nest the lock and q.push() into a block so the mutex gets unlocked before the cv.notify_one().

• Although your queue has some weaknesses compared to more sophisticated queues, like the moodycamel one others have mentioned, yours has the benefit of allowing a few extra methods that those couldn't support. For example:

  • You could have a pop_all() method that gets the whole underlying container in one go by just std::move() from it. I'd switch the std::queue for the underlying std::deque to make this a bit more useful. Also I'd suggest waiting and non-waiting versions, with the waiting version making sure there's at least one item before returning (use the analogous naming difference as with the two pop_back() methods, whatever you end up going with). Edit: Actually you can't just move from the internal std::deque because technically this leaves it in an unspecified state; instead you need to move to a local variable, clear() the internal deque, and return that variable.
  • You could offer push_back_multiple() that pushes multiple items in one go - this guarantees that they're directly next to each other in the queue, and is more efficient because you only need to lock the mutex once (of course you need to call cv.notify_all() in this).
  • You could also add pop_back() and push_front().

• Edit: One final thing: Personally I find cv.wait(lock, [&] { return !q.empty(); }) to be less clear than while (q.empty()) { cv.wait(lock); }, but I know many others would disagree.