# Have I thought of everything in this wrapper around boost::thread_group?

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.)

#ifndef HASTHREADS_H

#include <set>
#include <iostream>
#include <boost/function.hpp>
#include <boost/shared_ptr.hpp>

/**
* 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
*/
{
public:

/**
*
* for what this actually means.
*/

/**
* Wait for all pending threads to terminate.
*/

protected:

/**
* Create and run a tracked thread.
*/
template <typename Callable>
void createThread(Callable f, bool allowSignals = false);

/**
* Blocks SIGINT, SIGTERM and SIGQUIT signals in the current thread.
* Should always be used in a new thread, since we have a dedicated
* signal handling thread that we want to always receive any of these
* signals.
*/

/**
* Returns the number of threads currently managed by this object.
*/

private:

/**
* Entrypoint function for a thread.
* Wraps the user-provided worker function in signal and exception handling.
*/
template <typename Callable>

size_t count;

mutable boost::mutex count_lock;
};

template <typename Callable>
{
{
// Although boost::thread_group does a good job of looking after threads for us,
// it doesn't auto-remove objects wrapping terminated threads. So we must do that... carefully.

// We need tp to be valid *before* the thread starts, so that its third argument is
// always guaranteed to be valid. But we also need tp to be the pointer we put into the
// thread_group. So we swap a running thread object into the original non-running one.

tp->swap(tmp);

{
boost::mutex::scoped_lock l(count_lock);
count++;
}
}
}

template <typename Callable>
{
if (!allowSignals)

{
// Wait until createThread's work with this thread object is definitely done
}

try {
f();
}

// Yes, we should catch this exception! Letting it bubble over is _potentially_ dangerous:
// http://stackoverflow.com/questions/6375121

std::cout << "Thread " << boost::this_thread::get_id() << " interrupted (and ended)." << std::endl;
}
catch (std::exception& e) {
std::cout << "Exception caught from thread " << boost::this_thread::get_id() << ": " << e.what() << std::endl;
}
catch (...) {
std::cout << "Unknown exception caught from thread " << boost::this_thread::get_id() << std::endl;
}

// would block as they sit on the same mutex. Silly >.< So we check a "removal" flag first.
{
if (!l) // can only occur when has_threads::waitForThreads() is in progress and we shouldn't be self-deleting
return;

{
boost::mutex::scoped_lock l(count_lock);
count--;
}

delete tp;
}
}

#endif


#include "has-threads.h"
#include <boost/foreach.hpp>
#include <csignal>

#include <iostream>

: grp(NULL), count(0)
{
}

{
delete grp;
}

{
grp->interrupt_all();
}

{
// Let any thread initialisation finish, otherwise
// we might see deadlock on thread_create_lock.

{
// Protects from threads being created whilst we join everything we already have.

// Protects from threads in the process of destroying themselves
// from conflicting with the following logic.

grp->join_all();

// Now reset the group, so that any thread objects whose threads ended
// during that join_all are destroyed properly.
delete grp;
}
}

{
boost::mutex::scoped_lock l(count_lock);
return count;
}

{
sigset_t signal_set;
sigemptyset(&signal_set);
}

-
Note: Yes, I'm aware that thread pools are "better" in a lot of cases. But I do want to be able to just create and destroy threads at will. –  Lightness Races in Orbit Feb 26 '12 at 0:21
And, yes, all this locking makes me think "gosh, there must be a simpler way", but I don't think that there is. Perhaps you can point out something I've missed. –  Lightness Races in Orbit Mar 6 '12 at 14:46

I think you have UB here: 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).
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.