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.

diff --git a/src/platform.h b/src/platform.h
index e4b4d2ceaef144b90b2f6a40a159b1b4a7f23f6f..206ee4e9077171e1d6d703dff5b0fc83f8c0abb3 100644 (file)
--- 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 2b09a69fad957307932a7ed427ea66d285e95c52..25ef1853c03755762db7645771ffdccfb599ff26 100644 (file)
--- a/src/thread.cpp
+++ b/src/thread.cpp
@@ -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();
   }
 }

diff --git a/src/thread.h b/src/thread.h
index 4ad08465ff30751ae3f89777ac57ff068d2485d5..f5135804710b4003c06cbffaef24c73119d294af 100644 (file)
--- 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();