#lock_guard

#queue

#log_task

#main

#General stuff

#Q & A

lock_guard

queue

log_task

main

General stuff

Q & A

• The destructor never checks if mtx == nullptr, which will cause problems if lock_guard got default-constructed.

• The lock_guard(M&) constructor cannot be noexcept, as lock on a standard library compatible mutex class is allowed to throw.

• The destructor never checks if mtx == nullptr, which will cause problems if lock_guard got default-constructed.

• The lock_guard(M&) constructor cannot be noexcept, as lock on a standard library compatible mutex class is allowed to throw.

  However, it could be made conditionally noexcept in case M::lock is noexcept itself

   lock_guard(M& m) noexcept(noexcept(std::declval<M>().lock())) : mtx{&m} { mtx->lock(); }`
  

• lock_guard(M&) should be made explicit in order to prevent unwanted conversions.

• Memory leak: push(const T&) never deletes new_elem.

  Actually, why allocate new_elem at all? The whole first 4 lines could be simplified to:

   auto node = new queue_node{ elem, nullptr };
  

• push(const T&=) and pop() may cause undefined behavior by calling one_elem_mtx.unlock() if it hasn't been locked before.

• pop() doesn't update tail in case the last element got removed and lets it dangle instead.

  This will cause undefined behavior upon the next call to push(const T&).

  This also means that one_elem_mtx will not be locked in calls to push(const T&) or pop(), since head != tail while the queue is empty again.

• Race condition: one_elem_mtx needs to be acquired after the lock on pop_mtx/push_mtx.

  Reason: Assume we have a queue with two elements in it, and two threads A and B who both want to execute pop. Thread A executes until just after if(head == tail) (which of course right now evaluates to false) and then gets interrupted by the OS. Thread B runs pop() to completion in the meantime, leaving the queue at one object. Now, assuming the missing tail update mentioned above gets added, we have a potential data race on access to tail if another Thread C were to run push(const T&).

• Thread starvation: Once the race condition is fixed, if pop() gets called much more frequently than push(const T&), threads waiting on pop() might starve threads trying to push(const T&) from getting access to one_elem_mtx. Maybe make pop a blocking operation (using a std::condition_variable to notify if new elements got inserted)?

• For a production ready queue, you might want to think about adding a maximum capacity (so the queue doesn't grow too large if elements get added faster than they get removed).

• An overload push(T&&) might be nice.

• Memory leak: push(const T&) never deletes new_elem.

  Actually, why allocate new_elem at all? The whole first 4 lines could be simplified to:

   auto node = new queue_node{ elem, nullptr };
  

• push(const T&) and pop() may cause undefined behavior by calling one_elem_mtx.unlock() if it hasn't been locked before.

• pop() doesn't update tail in case the last element got removed and lets it dangle instead.

  This will cause undefined behavior upon the next call to push(const T&).

  This also means that one_elem_mtx will not be locked in calls to push(const T&) or pop(), since head != tail while the queue is empty again.

• Race condition: one_elem_mtx needs to be acquired after the lock on pop_mtx/push_mtx.

  Reason: Assume we have a queue with two elements in it, and two threads A and B who both want to execute pop. Thread A executes until just after if(head == tail) (which of course right now evaluates to false) and then gets interrupted by the OS. Thread B runs pop() to completion in the meantime, leaving the queue at one object. Now, assuming the missing tail update mentioned above gets added, we have a potential data race on access to tail if another Thread C were to run push(const T&).

• Thread starvation: Once the race condition is fixed, if pop() gets called much more frequently than push(const T&), threads waiting on pop() might starve threads trying to push(const T&) from getting access to one_elem_mtx. Maybe make pop a blocking operation (using a std::condition_variable to notify if new elements got inserted)?

• For a production ready queue, you might want to think about adding a maximum capacity (so the queue doesn't grow too large if elements get added faster than they get removed).

• An overload push(T&&) might be nice.