Problems with conditions in boost::thread

Hi, I just started to use boost::thread in a bigger application. Since the condition variables do not work as expected - e.g. the waiting thread never wakes up ( example can be found here: http://svn.zynot.org/svn/zynot/projects/xeta/trunk/source/src/xeta/curl.cpp ) - I started writing a smaller test case to see if I use the library correctly. That little test application works fine if I run it in with the gnu debugger, and if I have debugging symbols in my glibc. In all other cases the process gets terminated before any debuging print outs show up or the debugger fails to run the application: #include <list> #include <iostream> #include <boost/bind.hpp> #include <boost/thread/xtime.hpp> #include <boost/thread/thread.hpp> using namespace boost; struct Mini { bool running; thread worker; mutex list_mutex; condition items_empty; std::list<std::string> items; typedef mutex::scoped_lock lock; Mini(); ~Mini(); void run(); void add( std::string const& item); }; Mini::Mini() : worker( bind( &Mini::run, this ) ) { } Mini::~Mini() { running = false; { lock the_list( list_mutex ); items_empty.notify_one(); } worker.join(); } void Mini::run() { running = true; while( running ) { lock the_list( list_mutex ); if( running && items.empty() ) { std::cout << "Waiting for item" << std::endl; items_empty.wait( the_list ); std::cout << "Continue" << std::endl; } std::cout << "Removing :" << items.front() << std::endl; items.pop_front(); } } void Mini::add( std::string const& item ) { lock list_access( list_mutex ); std::cout << "Adding :" << item << std::endl; items.push_back( item ); std::cout << "Notifying other thread" << std::endl; items_empty.notify_one(); } int main() { Mini my_mini; my_mini.add("Once upon a time"); my_mini.add("there was a little mouse"); my_mini.add("that lived in a little house"); xtime xt; xtime_get(&xt, TIME_UTC); xt.sec += 2; thread::sleep(xt); } After that I tried to do it without boost. This time everything works as expected, with and without debugger. #include <list> #include <unistd.h> #include <iostream> #include <pthread.h> struct Mini { bool running; pthread_t worker; pthread_mutex_t mutex; pthread_cond_t not_empty; std::list<std::string> items; Mini(); ~Mini(); static void * call_run( void *); void run(); void add( std::string const& item); }; void* Mini::call_run( void * ptr) { static_cast<Mini*>(ptr)->run(); return 0; } Mini::Mini() { pthread_mutex_t temp_mut = PTHREAD_MUTEX_INITIALIZER; mutex = temp_mut; pthread_cond_t temp_cond = PTHREAD_COND_INITIALIZER; not_empty = temp_cond; worker = pthread_create( &worker, 0, Mini::call_run, this ); } Mini::~Mini() { running = false; { pthread_mutex_lock( &mutex ); pthread_cond_broadcast( ¬_empty ); pthread_mutex_unlock( &mutex ); } pthread_join( worker, 0 ); pthread_cond_destroy( ¬_empty ); } void Mini::run() { running = true; while( running ) { pthread_mutex_lock( & mutex ); if( running && items.empty() ) { std::cout << "Waiting for item" << std::endl; pthread_cond_wait(¬_empty, &mutex); std::cout << "Continue" << std::endl; } std::cout << "Removing :" << items.front() << std::endl; items.pop_front(); pthread_mutex_unlock( &mutex ); } } void Mini::add( std::string const& item ) { pthread_mutex_lock( & mutex ); std::cout << "Adding :" << item << std::endl; items.push_back( item ); std::cout << "Notifying other thread" << std::endl; pthread_cond_broadcast(& not_empty); pthread_mutex_unlock( &mutex ); } int main() { Mini my_mini; my_mini.add("Once upon a time"); my_mini.add("there was a little mouse"); my_mini.add("that lived in a little house"); sleep(2 ); } Any ideas?

Andreas Pokorny <diemumiee@gmx.de> wrote: []
struct Mini { bool running; thread worker; mutex list_mutex; condition items_empty; std::list<std::string> items;
typedef mutex::scoped_lock lock;
Mini(); ~Mini(); void run(); void add( std::string const& item); };
Mini::Mini() : worker( bind( &Mini::run, this ) ) { }
The constructor as it is written now is not safe. You start the thread before members list_mutex, items_emptym and items get initialized as they are declared later than worker member. It may be that the thread starts executing "too early" and accesses the uninitialized members. I'm not sure if that is the case but at least rearranging the members so that worker member gets declared last will certainly make it safer. -- Maxim Yegorushkin

Maxim Yegorushkin wrote:
Andreas Pokorny <diemumiee@gmx.de> wrote:
[]
struct Mini { bool running; thread worker; mutex list_mutex; condition items_empty; std::list<std::string> items;
typedef mutex::scoped_lock lock;
Mini(); ~Mini(); void run(); void add( std::string const& item); };
Mini::Mini() : worker( bind( &Mini::run, this ) ) { }
The constructor as it is written now is not safe. You start the thread before members list_mutex, items_emptym and items get initialized as they are declared later than worker member. It may be that the thread starts executing "too early" and accesses the uninitialized members. I'm not sure if that is the case but at least rearranging the members so that worker member gets declared last will certainly make it safer.
Yes! Thank you! That solved all my problems. Andreas Pokorny
participants (2)
-
Andreas Pokorny
-
Maxim Yegorushkin