patterncppMinor
Have I thought of everything in this wrapper around boost::thread_group?
Viewed 0 times
thisaroundthread_groupthoughtwrappereverythingboosthave
Problem
boost::thread_group doesn't automatically clean up contained threads when they end, so I needed something like that described in this boost-users list post. However, in my opinion that code is not safe, because there's no guarantee that tp will be .reset before the thread reaches its th_ptr.get() call.After an initial attempt at re-inventing
boost::thread_group entirely (which failed), I came up with this wrapper and, after much sweat, I believe it's complete and correct. It seems to fix every deadlock that was generally reproducible during its development, so I think I've got them all.Of course, I'm not sure.
What do you think of my wrapper?
(It also performs some signalling sanity — I only want signals to be handled in the "root"/parent thread, otherwise all hell breaks loose.)
has-threads.h
```
#ifndef HASTHREADS_H
#define HASTHREADS_H
#include
#include
#include
#include
#include
/**
* Concept to be derived by any class that should be capable of launching and tracking
* worker threads, and waiting for workers to complete at object destruction.
*
* Non-copyable.
*
*
* NOTE REGARDING DESTRUCTION
*
* Threads are automatically waited for on destruction, but they are not interrupted. So:
* - Deriving classes should call interruptThreads() if any are persistent and would not terminate
* soon on their own.
* - They should also call waitForThreads() if any callback uses member data, to ensure that
* the thread finished execution before the member data is destroyed.
*
*
* ANOTHER NOTE
*
* Be careful not to expose boost::thread_group::size(); if a thread uses it, this could cause
* a deadlock with boost::thread_group::join_all(). Instead, we can track it ourselves, safely.
*/
struct has_threads
{
public:
/**
* Asks all pending threads to terminate.
*
* See http://www.boost.org/doc/libs/1_40_0/doc/html/thread/thread_management.html
* for what this actu
Solution
I think you have UB here:
Your code is sufficiently complex for me not to be sure whether you have this type of UB.
However, if you avoid explicit calls to
mutex::unlock() must only be called (1) if the mutex is presently locked and (2) by the same thread that lock()ed it in the first place (i.e. if the calling thread owns the mutex). Anything else is UB (at least for std::mutex, but I suspect also for boost::mutex).Your code is sufficiently complex for me not to be sure whether you have this type of UB.
However, if you avoid explicit calls to
mutex::lock() and mutex::unlock() but instead use RAII via lock_guard<>, unique_lock<>, or shared_lock<>, you should be on the safe side.Context
StackExchange Code Review Q#9431, answer score: 6
Revisions (0)
No revisions yet.