Fix a race at thread creation
authorMarco Costalba <mcostalba@gmail.com>
Wed, 31 Jul 2013 04:59:24 +0000 (06:59 +0200)
committerMarco Costalba <mcostalba@gmail.com>
Wed, 31 Jul 2013 16:35:32 +0000 (18:35 +0200)
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
src/thread.cpp
src/thread.h

index e4b4d2c..206ee4e 100644 (file)
@@ -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
index 2b09a69..25ef185 100644 (file)
@@ -19,7 +19,6 @@
 
 #include <algorithm> // For std::count
 #include <cassert>
-#include <iostream>
 
 #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<typename T> 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<TimerThread>();
+  push_back(new_thread<MainThread>());
   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<Thread>());
 
   while (size() > requested)
   {
-      delete back();
+      delete_thread(back());
       pop_back();
   }
 }
index 4ad0846..f513580 100644 (file)
@@ -94,7 +94,7 @@ struct SplitPoint {
 struct Thread {
 
   Thread();
-  virtual ~Thread();
+  virtual ~Thread() {}
 
   virtual void idle_loop();
   void notify_one();