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 <boost/config.hpp> #include <boost/thread.hpp> #include <boost/functional.hpp> class WorkerThread { public: typedef boost::function<void ()> JobType; WorkerThread() { m_Thread = boost::thread(boost::bind(&WorkerThread::run, boost::ref(this))); m_Running = true; } ~WorkerThread() { m_Thread.join(); } const bool isRunning() const { return m_Running; } private: volatile bool m_Running; boost::thread m_Thread; void run() { m_Running = false; // Access violation here } private: WorkerThread(const WorkerThread& rhs); WorkerThread& operator = (const WorkerThread& rhs); }; int main() { WorkerThread wt; // new thread should start executing here return 0; } -Rob Yull
2009/7/29 Rob Yull <ryull@kaladorstudios.com>
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 <cdm.henderson@googlemail.com>
2009/7/29 Rob Yull <ryull@kaladorstudios.com>
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<void ()> 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
2009/7/29 Craig Henderson <cdm.henderson@googlemail.com>
class WorkerThread { public: typedef boost::function<void ()> 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 <cdm.henderson@googlemail.com> class WorkerThread { public: typedef boost::function<void ()> 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
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 <ilyasokol@gmail.com> wrote:
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