Race condition with Boost::Thread
I've been writing a thread pool class as kind of an experiment with
boost.threads, boost.bind, and boost.function. The code below is the worker
thread, that will eventually pull jobs from a queue, execute them, then look
and see if there is another job in the queue. I think I've got an
interesting race condition. The problem I'm having is when the class goes
out of scope. I'm getting an access violation from the run() member
function (which is function being run by m_Thread). Right now, run() is
just entering and leaving (no actual queue checking). By the time run() is
getting executed, the class is starting to execute the destructor. I
thought that having the join() call in the destructor should allow the
thread to finish before the class data is destroyed. In Visual Studio
2005's debugger, when it is getting to run(), it looks like the object has
already been destroyed (this = 0x7fffffff), so the access violation is
coming from the assignment to the m_Running member variable. If I take out
the assignment, and run the debug, when it is entering run(), the this
pointer is still showing as invalid, but the function completes and ends up
back at the join() in the destructor, where the this pointer in the
destructor is valid again. So as far as the destructor seems to be
conserned, the class hasn't been destroyed yet, but as far as the member
class is, it has been destroyed already.
If I do pad some instructions/time before the class goes out of scope, run()
executes just fine (this pointer is valid). I have also tried moving the
creation of m_Thread outside of the constructor to a separate member
function call, but the result is the same.
I know debugging multithreaded apps can be difficult. Is there right out
something I'm doing wrong here or making some kind of bad assumpion about
the execution of run()?
#include
2009/7/29 Rob Yull
I’ve been writing a thread pool class as kind of an experiment with boost.threads, boost.bind, and boost.function. The code below is the worker thread, that will eventually pull jobs from a queue, execute them, then look and see if there is another job in the queue. I think I’ve got an interesting race condition. The problem I’m having is when the class goes out of scope. I’m getting an access violation from the run() member function (which is function being run by m_Thread). Right now, run() is just entering and leaving (no actual queue checking). By the time run() is getting executed, the class is starting to execute the destructor. I thought that having the join() call in the destructor should allow the thread to finish before the class data is destroyed. In Visual Studio 2005’s debugger, when it is getting to run(), it looks like the object has already been destroyed (this = 0x7fffffff), so the access violation is coming from the assignment to the m_Running member variable. If I take out the assignment, and run the debug, when it is entering run(), the this pointer is still showing as invalid, but the function completes and ends up back at the join() in the destructor, where the this pointer in the destructor is valid again. So as far as the destructor seems to be conserned, the class hasn’t been destroyed yet, but as far as the member class is, it has been destroyed already.
If I do pad some instructions/time before the class goes out of scope, run() executes just fine (this pointer is valid). I have also tried moving the creation of m_Thread outside of the constructor to a separate member function call, but the result is the same.
The WorkerThread object is going out of scope before the run() is executed in the secondary thread. You need to synchronise the startup/shutdown down. I suggest you move the m_Running=true; into the run() [the thread isn't running until run() is called, afterall], and in the dtor, wait for the thread to start before calling join. void run() { BOOST_ASSERT(m_Running); } Regards -- Craig
2009/7/29 Craig Henderson
2009/7/29 Rob Yull
I’ve been writing a thread pool class as kind of an experiment with boost.threads, boost.bind, and boost.function. The code below is the worker thread, that will eventually pull jobs from a queue, execute them, then look and see if there is another job in the queue. I think I’ve got an interesting race condition. The problem I’m having is when the class goes out of scope. I’m getting an access violation from the run() member function (which is function being run by m_Thread). Right now, run() is just entering and leaving (no actual queue checking). By the time run() is getting executed, the class is starting to execute the destructor. I thought that having the join() call in the destructor should allow the thread to finish before the class data is destroyed. In Visual Studio 2005’s debugger, when it is getting to run(), it looks like the object has already been destroyed (this = 0x7fffffff), so the access violation is coming from the assignment to the m_Running member variable. If I take out the assignment, and run the debug, when it is entering run(), the this pointer is still showing as invalid, but the function completes and ends up back at the join() in the destructor, where the this pointer in the destructor is valid again. So as far as the destructor seems to be conserned, the class hasn’t been destroyed yet, but as far as the member class is, it has been destroyed already.
If I do pad some instructions/time before the class goes out of scope, run() executes just fine (this pointer is valid). I have also tried moving the creation of m_Thread outside of the constructor to a separate member function call, but the result is the same.
[sorry for the partial post earlier]
Hi Rob,
The WorkerThread object is going out of scope before the run() is executed
in the secondary thread. You need to synchronise the startup/shutdown down.
I suggest you move the m_Running=true; into the run() [the thread isn't
running until run() is called, afterall], and in the dtor, wait for the
thread to start before calling join.
class WorkerThread
{
public:
typedef boost::function
2009/7/29 Craig Henderson
class WorkerThread { public: typedef boost::function
JobType; WorkerThread() { m_Thread = boost::thread(boost::bind(&WorkerThread::run, boost::ref(this))); }
~WorkerThread() { while (!m_Running) ; m_Thread.join(); m_Running = false; }
const bool isRunning() const { return m_Running; }
private: volatile bool m_Running; boost::thread m_Thread;
void run() { m_Running = true; BOOST_ASSERT(isRunning()); }
private: WorkerThread(const WorkerThread& rhs); WorkerThread& operator = (const WorkerThread& rhs); };
Regards -- Craig
You need to initial m_Running=false; in the ctor, too. -- Craig
That all makes sense and seemed to fix the problem. Thanks for the fast
reply.
-Rob Yull
From: boost-users-bounces@lists.boost.org
[mailto:boost-users-bounces@lists.boost.org] On Behalf Of Craig Henderson
Sent: Wednesday, July 29, 2009 10:47 AM
To: boost-users@lists.boost.org
Subject: Re: [Boost-users] Race condition with Boost::Thread
2009/7/29 Craig Henderson
Rob Yull wrote:
WorkerThread() { m_Thread = boost::thread(boost::bind(&WorkerThread::run, boost::ref(this)));
It should not compile, you need m_Thread = boost::thread(boost::bind(&WorkerThread::run, this)); or simply m_Thread = boost::thread(&WorkerThread::run, this);
m_Running = true; }
Hi guys,
This threaded code can be improved further:
===============
~WorkerThread() {
while (!m_Running)
;
m_Thread.join();
m_Running = false;
}
================
Endlessly polling the m_Running with an empty while loop is inefficient. It
would be better to use the boost::condition and boost::mutex for thread
synchronization [the 2nd thread notifies the 1st (main() ) one when it
completes its task]. A possible improvement can be achieved like this:
--------------Begin-------------
boost::condition taskCompleted;
boost::mutex taskMutex;
class WorkerThread { // class definition
...
~WorkerThread() {
boost::mutex::scoped_lock lock(taskMutex);
{
taskCompleted.wait(lock); // wait for notification
m_Thread.join();
}
...
void run() {
boost::mutex::scoped_lock lock(taskMutex);
{
// work to do
// after work is done
taskCompleted.notify_one();// I am done, your turn now.
} // lock scope
} run() scope
}; // class definition
--------------end-------------
The volatile bool m_Running variable, its initialization and assignments,
and const bool isRunning() const function in the original code are no longer
needed.
Cheers,
Robert
On Thu, Jul 30, 2009 at 7:33 AM, Ilya Sokolov
Rob Yull wrote:
WorkerThread() { m_Thread = boost::thread(boost::bind(&WorkerThread::run, boost::ref(this)));
It should not compile, you need
m_Thread = boost::thread(boost::bind(&WorkerThread::run, this));
or simply
m_Thread = boost::thread(&WorkerThread::run, this);
m_Running = true;
}
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
participants (4)
-
Boost lzw -
Craig Henderson -
Ilya Sokolov -
Rob Yull