«

I am trying to make a thread object, so I can later have better
control of threads and the resources they are using. So, I started
wrapping up a boost::thread.

//---------------------------------------------------------------------------------------------------------------------------------------------
// BaseThread.h
#include <boost/thread/thread.hpp>

//------------------------------------------------------------------------------
/// \brief Thread object that performs a task that is to run once (non-
looped)
/// \detail Derive from this class and implement the Work method to
define the
/// work this thread is to perform
class BaseThread
{
public:

BaseThread();

/// \brief Deconstructor
/// \detail It is required that a derived class make a call to
thread_.join()
/// in its deconstructor
virtual ~BaseThread();

virtual void Start();
virtual int Work() = 0;

protected:

void Run();

boost::thread thread_;
};

//---------------------------------------------------------------------------------------------------------------------------------------------
// BaseThread.cpp

#include "BaseThread.h"


//------------------------------------------------------------------------------
BaseThread::BaseThread()
{
}

//------------------------------------------------------------------------------
BaseThread::~BaseThread()
{
// Wait for the thread to complete
thread_.join();

// DEBUG - Does join wait?
int x = 1;
}

//------------------------------------------------------------------------------
void BaseThread::Start()
{
boost::thread newThread(boost::bind(&BaseThread::Run, this));
thread_.swap(newThread);
}

//------------------------------------------------------------------------------
void BaseThread::Run()
{
this->Work();
}


I am kind of winging it here just going off of the boost documentation
and guessing at what I should be doing. This code has a problem in
that in order for us to wait for the thread to complete, the derived
class is going to have to call thread_.join(). That leaves a bad taste
in my mouth as I'd rather the base class take care of all the details,
and all the derived class has to worry about is providing a Work
method.

This class doesn't provide much that boost::thread doesn't already
have, but I want to make different classes for additonal requirements.
One of which I have in mind is a thread with looped work and a start,
stop, method.

Making an object out of it will also probably help with derving
classes that take care of locking their data...or at least organize it
a bit.

So the question is, is there a way I can avoid the requirement here
that derived classes call thread_.join() and do something different in
the base class?

I'll probably have more questions as I progress.

Maybe I'm missing something, but why would a derived class need to call thread_.join()? You have it in your base destructor and base destructors are automatically called after the derived destructor executes.

http://www.parashift.com/c++-faq-lit...html#faq-11.12

Sorry if this is a repost... my Google Groups doesn't seem to be loading my post from yesterday.

Maybe I'm missing something but, why is it a requirement for derived classes to call thread_.join()? You have it in the destructor of your base class, and this will be called after the derived class' destructor executes.

http://www.parashift.com/c++-faq-lit...html#faq-11.12

Sorry if this is a repost, google groups doesn't seem to be posting (or at least displaying) my messages from yesterday.

Maybe I'm missing something but why is having your derived class call thread_.join() a requirement? You have it in your base class' dtor and this will be called when derived objects are destroyed.

http://www.parashift.com/c++-faq-lit...html#faq-11.12

On Aug 1, 7:20 pm, Christopher <cp...@austin.rr.com> wrote:
> I am trying to make a thread object, so I can later have
> better control of threads and the resources they are using.
> So, I started wrapping up a boost::thread.

> //---------------------------------------------------------------------------------------------------------------------------------------------
> // BaseThread.h
> #include <boost/thread/thread.hpp>

> //------------------------------------------------------------------------------
> /// \brief Thread object that performs a task that is to run once (non- looped)
> /// \detail Derive from this class and implement the Work method to define the
> /// work this thread is to perform

This is *not* a good way to go about it. The class containing
the code to be executed should be separate from the thread
object itself. (You might want to pass it as an argument to the
constructor of the thread object, however.)

> class BaseThread
> {
> public:
> BaseThread();

> /// \brief Deconstructor
> /// \detail It is required that a derived class make a call to
> thread_.join()
> /// in its deconstructor
> virtual ~BaseThread();

> virtual void Start();
> virtual int Work() = 0;

> protected:

> void Run();

> boost::thread thread_;
> };

> //---------------------------------------------------------------------------------------------------------------------------------------------
> // BaseThread.cpp

> #include "BaseThread.h"

> //------------------------------------------------------------------------------
> BaseThread::BaseThread()
> {
> }

> //------------------------------------------------------------------------------
> BaseThread::~BaseThread()
> {
> // Wait for the thread to complete
> thread_.join();

I'm afraid I don't like this. I don't like the idea of a class
waiting for anything in its destructor; it can cause the code to
hang in unexpected places.

If the threads are to be joinable, then it is the client who
should do the joining, not the thread class. (But threads don't
have to be joinable.)

> // DEBUG - Does join wait?
> int x = 1;
> }

> //------------------------------------------------------------------------------
> void BaseThread::Start()
> {
> boost::thread newThread(boost::bind(&BaseThread::Run, this));
> thread_.swap(newThread);
> }

> //------------------------------------------------------------------------------
> void BaseThread::Run()
> {
> this->Work();
> }

> I am kind of winging it here just going off of the boost documentation
> and guessing at what I should be doing.

It isn't. Although Boost doesn't make the distinction it really
should. Threads may be joinable or not. In the case of Boost,
if you call the destructor of a boost::thread before joining,
the thread will be detached, and will go along on its merry way.
This may or may not be what you wanted; if the thread is
supposed to be joinable, and an exception causes the destructor
to be called (and the context where the join was to take place)
to disappear, you really want to somehow abort the thread. If
the thread operates more in fire and forget mode (e.g. a thread
started from the GNI thread, in response to a GUI event), then
you don't want it to be joinable (and doing a join from the
context were the thread was started defeats the purpose of
starting it), there's really nothing for the thread object to
do, and no point in even having it. Just drop the boost::thread
object as soon as the thread has been started.

--
James Kanze

On Aug 7, 1:18*pm, James Kanze <james.ka...@gmail.com> wrote:
> On Aug 1, 7:20 pm, Christopher <cp...@austin.rr.com> wrote:
>
> > I am trying to make a thread object, so I can later have
> > better control of threads and the resources they are using.
> > So, I started wrapping up a boost::thread.
> > //-------------------------------------------------------------------------*--------------------------------------------------------------------
> > // BaseThread.h
> > #include <boost/thread/thread.hpp>
> > //-------------------------------------------------------------------------*-----
> > /// \brief Thread object that performs a task that is to run once (non-looped)
> > /// \detail Derive from this class and implement the Work method to define the
> > /// * * * * work this thread is to perform
>
> This is *not* a good way to go about it. *The class containing
> the code to be executed should be separate from the thread
> object itself. *(You might want to pass it as an argument to the
> constructor of the thread object, however.)
>
>
>
>
>
> > class BaseThread
> > {
> > public:
> > * * BaseThread();
> > * * /// \brief Deconstructor
> > * * /// \detail It is required that a derived class make a call to
> > thread_.join()
> > * * /// * * * * in its deconstructor
> > * * virtual ~BaseThread();
> > * * virtual void Start();
> > * * virtual int Work() = 0;
> > protected:
> > * * void Run();
> > * * boost::thread thread_;
> > };
> > //-------------------------------------------------------------------------*--------------------------------------------------------------------
> > // BaseThread.cpp
> > #include "BaseThread.h"
> > //-------------------------------------------------------------------------*-----
> > BaseThread::BaseThread()
> > {
> > }
> > //-------------------------------------------------------------------------*-----
> > BaseThread::~BaseThread()
> > {
> > * * // Wait for the thread to complete
> > * * thread_.join();
>
> I'm afraid I don't like this. *I don't like the idea of a class
> waiting for anything in its destructor; it can cause the code to
> hang in unexpected places.
>
> If the threads are to be joinable, then it is the client who
> should do the joining, not the thread class. *(But threads don't
> have to be joinable.)
>
>
>
>
>
> > * * // DEBUG - Does join wait?
> > * * int x = 1;
> > }
> > //-------------------------------------------------------------------------*-----
> > void BaseThread::Start()
> > {
> > * * boost::thread newThread(boost::bind(&BaseThread::Run, this));
> > * * thread_.swap(newThread);
> > }
> > //-------------------------------------------------------------------------*-----
> > void BaseThread::Run()
> > {
> > * * this->Work();
> > }
> > I am kind of winging it here just going off of the boost documentation
> > and guessing at what I should be doing.
>
> It isn't. *Although Boost doesn't make the distinction it really
> should. *Threads may be joinable or not. *In the case of Boost,
> if you call the destructor of a boost::thread before joining,
> the thread will be detached, and will go along on its merry way.
> This may or may not be what you wanted; if the thread is
> supposed to be joinable, and an exception causes the destructor
> to be called (and the context where the join was to take place)
> to disappear, you really want to somehow abort the thread. *If
> the thread operates more in fire and forget mode (e.g. a thread
> started from the GNI thread, in response to a GUI event), then
> you don't want it to be joinable (and doing a join from the
> context were the thread was started defeats the purpose of
> starting it), there's really nothing for the thread object to
> do, and no point in even having it. *Just drop the boost::thread
> object as soon as the thread has been started.
>
> --
> James Kanze- Hide quoted text -
>
> - Show quoted text -- Hide quoted text -
>
> - Show quoted text -


The more I am reading, the more I am coming to that conclusion that
the work the thread is to do should be seperated from the thread
object itself, and that was one of the goals in the boost::thread
implementation.

I suppose I could follow that principle as well.
My main goals are to provide a start and stop mechanism, and also to
provide a distiction between a thread that does work one time and dies
as compared to a thread that loops until some condition occurs.

I wonder, if I am to seperate the work, and take it in the
constructor, what form the work will take. I need to read more on
boost::bind I think.

On Aug 7, 11:18*am, James Kanze <james.ka...@gmail.com> wrote:
> On Aug 1, 7:20 pm, Christopher <cp...@austin.rr.com> wrote:
>
> > class BaseThread
> > {
> > public:
> > * * BaseThread();
> > * * /// \brief Deconstructor
> > * * /// \detail It is required that a derived class make a call to
> > thread_.join()
> > * * /// * * * * in its deconstructor
> > * * virtual ~BaseThread();
> > * * virtual void Start();
> > * * virtual int Work() = 0;
> > protected:
> > * * void Run();
> > * * boost::thread thread_;
> > };
> > //---------------------------------------------------------------------------------------------------------------------------------------------
> > // BaseThread.cpp
> > #include "BaseThread.h"
> > //------------------------------------------------------------------------------
> > BaseThread::BaseThread()
> > {
> > }
> > //------------------------------------------------------------------------------
> > BaseThread::~BaseThread()
> > {
> > * * // Wait for the thread to complete
> > * * thread_.join();
>
> I'm afraid I don't like this. *I don't like the idea of a class
> waiting for anything in its destructor; it can cause the code to
> hang in unexpected places.
>
> If the threads are to be joinable, then it is the client who
> should do the joining, not the thread class. *(But threads don't
> have to be joinable.)

Silly question. I've heard this several times now, but I don't quite
see how you would do it otherwise. Let me give a simple example. I
wrote an alternative to std::for_each called concurrent_for_each. It
takes a range, a functor/function (like std::for_each), and an number
of threads (optional). It applies the functor/function to each element
of the range just like for_each, except it applies it in some
unspecified order, possibly concurrently.

Specifically, the idea is to split the range into sub-ranges and give
each sub-range to a throwaway thread. (If I was feeling especially
fancy, I could stash the threads instead of remaking them for each
concurrent_for_each invocation.)

Now, the question is how to write the function. The only sensible
approach seems to be to give the weak exception guarantee, that if an
exception is thrown by an invocation of the functor/function, then the
range will be left in some consistent state, but it's not specified
which state. It would be hard to give anything stronger while allowing
concurrent execution and modification of elements in the range.

So, the concurrent_for_each function is pretty simple. It's divide the
range into subranges, create the N threads with a private main-functor
that applies the argument functor to elements in the subrange.

The problem happens when and if one of them throws. Alternatively,
what if a thread creation throws due to out of memory, or various
other fail conditions. Should you join on threads already created? I
would think yes. This seems like good style and resource management to
not simple forget about them. I've had several bugs from analogous
situations where presumably short lived processes were not joined /
wait'ed. I figure similar arguments apply to threads.

So, I could do a manual try block and join on all created threads, or
I could use a vector-like container which will call delete on its
contained pointers. The second approach appears to be the nicer one,
but this contradicts the advice you just gave. At least, I think it
does.

What do you suggest? Where have I gone wrong?