From 4d46d29efe4ee496bda2aa7ea83184d502944852 Mon Sep 17 00:00:00 2001 From: Marco Costalba Date: Wed, 31 Jul 2013 06:59:24 +0200 Subject: [PATCH 1/1] Fix a race at thread creation At thread creation start_routine() is called and from there the virtual function idle_loop() because we do this inside Thread c'tor, where the virtual mechanism is disabled, it could happen that the base class idle_loop() is called instead. The issue happens with TimerThread and MainThread where, at launch, start_routine calls Thread::idle_loop instead of the derived ones. Normally this bug is hidden because c'tor finishes before start_routine() is actually called in the just created execution thread, but on some platforms and in some cases this is not guaranteed and the engine hangs. Reported by Ted Wong on talkchess No functional change. --- src/platform.h | 4 ++-- src/thread.cpp | 53 +++++++++++++++++++++++++------------------------- src/thread.h | 2 +- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/platform.h b/src/platform.h index e4b4d2ce..206ee4e9 100644 --- a/src/platform.h +++ b/src/platform.h @@ -66,7 +66,7 @@ typedef void*(*pt_start_fn)(void*); # define cond_signal(x) pthread_cond_signal(&(x)) # define cond_wait(x,y) pthread_cond_wait(&(x),&(y)) # define cond_timedwait(x,y,z) pthread_cond_timedwait(&(x),&(y),z) -# define thread_create(x,f,t) !pthread_create(&(x),NULL,(pt_start_fn)f,t) +# define thread_create(x,f,t) pthread_create(&(x),NULL,(pt_start_fn)f,t) # define thread_join(x) pthread_join(x, NULL) #else // Windows and MinGW @@ -105,7 +105,7 @@ inline DWORD* dwWin9xKludge() { static DWORD dw; return &dw; } # define cond_signal(x) SetEvent(x) # define cond_wait(x,y) { lock_release(y); WaitForSingleObject(x, INFINITE); lock_grab(y); } # define cond_timedwait(x,y,z) { lock_release(y); WaitForSingleObject(x,z); lock_grab(y); } -# define thread_create(x,f,t) (x = CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)f,t,0,dwWin9xKludge()), x != NULL) +# define thread_create(x,f,t) (x = CreateThread(NULL,0,(LPTHREAD_START_ROUTINE)f,t,0,dwWin9xKludge())) # define thread_join(x) { WaitForSingleObject(x, INFINITE); CloseHandle(x); } #endif diff --git a/src/thread.cpp b/src/thread.cpp index 2b09a69f..25ef1853 100644 --- a/src/thread.cpp +++ b/src/thread.cpp @@ -19,7 +19,6 @@ #include // For std::count #include -#include #include "movegen.h" #include "search.h" @@ -30,14 +29,32 @@ using namespace Search; ThreadPool Threads; // Global object -namespace { extern "C" { +namespace { // start_routine() is the C function which is called when a new thread // is launched. It is a wrapper to the virtual function idle_loop(). - long start_routine(Thread* th) { th->idle_loop(); return 0; } + extern "C" { long start_routine(Thread* th) { th->idle_loop(); return 0; } } -} } + + // Helpers to launch a thread after creation and joining before delete. Must be + // outside Thread c'tor and d'tor because object shall be fully initialized + // when start_routine (and hence virtual idle_loop) is called and when joining. + + template T* new_thread() { + T* th = new T(); + thread_create(th->handle, start_routine, th); + return th; + } + + void delete_thread(Thread* th) { + th->exit = true; // Search must be already finished + th->notify_one(); + thread_join(th->handle); // Wait for thread termination + delete th; + } + +} // Thread c'tor starts a newly-created thread of execution that will call @@ -50,22 +67,6 @@ Thread::Thread() /* : splitPoints() */ { // Value-initialization bug in MSVC activeSplitPoint = NULL; activePosition = NULL; idx = Threads.size(); - - if (!thread_create(handle, start_routine, this)) - { - std::cerr << "Failed to create thread number " << idx << std::endl; - ::exit(EXIT_FAILURE); - } -} - - -// Thread d'tor waits for thread termination before to return - -Thread::~Thread() { - - exit = true; // Search must be already finished - notify_one(); - thread_join(handle); // Wait for thread termination } @@ -186,8 +187,8 @@ bool Thread::is_available_to(Thread* master) const { void ThreadPool::init() { sleepWhileIdle = true; - timer = new TimerThread(); - push_back(new MainThread()); + timer = new_thread(); + push_back(new_thread()); read_uci_options(); } @@ -196,10 +197,10 @@ void ThreadPool::init() { void ThreadPool::exit() { - delete timer; // As first because check_time() accesses threads data + delete_thread(timer); // As first because check_time() accesses threads data for (iterator it = begin(); it != end(); ++it) - delete *it; + delete_thread(*it); } @@ -217,11 +218,11 @@ void ThreadPool::read_uci_options() { assert(requested > 0); while (size() < requested) - push_back(new Thread()); + push_back(new_thread()); while (size() > requested) { - delete back(); + delete_thread(back()); pop_back(); } } diff --git a/src/thread.h b/src/thread.h index 4ad08465..f5135804 100644 --- a/src/thread.h +++ b/src/thread.h @@ -94,7 +94,7 @@ struct SplitPoint { struct Thread { Thread(); - virtual ~Thread(); + virtual ~Thread() {} virtual void idle_loop(); void notify_one(); -- 2.39.2