Merge pull request #1035 from leapmotion/fix-more-issues

Fix more nagging bugs

src/autowiring/BasicThread.cpp

@@ -41,7 +41,7 @@ void BasicThread::DoRun(std::shared_ptr<CoreObject>&& refTracker) {
 
   // Update the thread priority.  This value may have been assigned before we started.  In that case,
   // we want to be sure we get the correct value assigned eventually.
-  SetThreadPriority(m_priority);
+  UpdateThreadPriority(std::unique_lock<std::mutex>{ m_state->m_threadLock });
 
   // Now we wait for the thread to be good to go:
   try {
@@ -134,12 +134,15 @@ bool BasicThread::OnStart(void) {
   // enables us to decide in advance the exact location in memory where the
   // object will be stored.
   auto outstanding = GetOutstanding();
-  m_state->m_thisThread.~thread();
-  new (&m_state->m_thisThread) std::thread(
-    [this, outstanding] () mutable {
-      this->DoRun(std::move(outstanding));
-    }
-  );
+  {
+    std::lock_guard<std::mutex> lk(m_state->m_threadLock);
+    m_state->m_thisThread.~thread();
+    new (&m_state->m_thisThread) std::thread(
+      [this, outstanding] () mutable {
+        this->DoRun(std::move(outstanding));
+      }
+    );
+  }
   return true;
 }
 
@@ -180,6 +183,7 @@ bool BasicThread::DoAdditionalWait(std::chrono::nanoseconds timeout) {
 }
 
 std::shared_ptr<void> BasicThread::GetThread(void) const {
+  std::lock_guard<std::mutex> lk(m_state->m_threadLock);
   // Return an aliased shared pointer
   return {
     m_state,
@@ -205,3 +209,44 @@ void ForceCoreThreadReidentify(void) {
 bool BasicThread::IsMainThread(void) {
   return mainTID == std::this_thread::get_id();
 }
+
+ThreadPriority BasicThread::GetThreadPriority(void) {
+  std::lock_guard<std::mutex> lk(m_state->m_threadLock);
+  return m_state->m_priority;
+}
+
+void BasicThread::UpdateThreadPriority(std::unique_lock<std::mutex>&& lock) {
+  if (m_state->m_thisThread.get_id() == std::thread::id())
+    return;
+  SetThreadPriority(m_state->m_thisThread.native_handle(), m_state->m_priority);
+}
+
+ThreadPriority BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
+  std::unique_lock<std::mutex> lk(m_state->m_threadLock);
+  ThreadPriority prevThreadPriority = m_state->m_priority;
+  m_state->m_priority = threadPriority;
+  UpdateThreadPriority(std::move(lk));
+  return prevThreadPriority;
+}
+
+ThreadPriority BasicThread::ElevateThreadPriority(ThreadPriority threadPriority) {
+  std::unique_lock<std::mutex> lk(m_state->m_threadLock);
+  ThreadPriority prevThreadPriority = m_state->m_priority;
+  if (threadPriority < m_state->m_priority) {
+    return prevThreadPriority;
+  }
+  m_state->m_priority = threadPriority;
+  UpdateThreadPriority(std::move(lk));
+  return prevThreadPriority;
+}
+
+ThreadPriority BasicThread::DeelevateThreadPriority(ThreadPriority threadPriority) {
+  std::unique_lock<std::mutex> lk(m_state->m_threadLock);
+  ThreadPriority prevThreadPriority = m_state->m_priority;
+  if (threadPriority >= m_state->m_priority) {
+    return prevThreadPriority;
+  }
+  m_state->m_priority = threadPriority;
+  UpdateThreadPriority(std::move(lk));
+  return prevThreadPriority;
+}

src/autowiring/BasicThread.h

@@ -6,6 +6,7 @@
 #include FUNCTIONAL_HEADER
 #include MEMORY_HEADER
 #include MUTEX_HEADER
+#include THREAD_HEADER
 
 class BasicThread;
 class CoreContext;
@@ -93,25 +94,22 @@ class BasicThread:
     /// then the current thread priority. Destroy this ElevatePriority instance
     /// to restore the normal thread priority.
     ElevatePriority(BasicThread& thread, ThreadPriority priority) :
-      m_oldPriority(thread.m_priority),
-      m_thread(thread)
+      m_thread(thread),
+      // Elevate if the new level is greater than the current level:
+      m_oldPriority(thread.ElevateThreadPriority(priority))
     {
-      // Elevate if the new level is higher than the old level:
-      if (priority > m_oldPriority)
-        m_thread.SetThreadPriority(priority);
     }
 
     /// Destroying this object returns the thread to its previous priority
     /// level.
     ~ElevatePriority(void) {
-      // Delevate if the old level is lower than the current level:
-      if (m_thread.m_priority > m_oldPriority)
-        m_thread.SetThreadPriority(m_oldPriority);
+      // De-elevate if the old level is lower than the current level:
+      m_thread.DeelevateThreadPriority(m_oldPriority);
     }
 
   private:
-    ThreadPriority m_oldPriority;
     BasicThread& m_thread;
+    ThreadPriority m_oldPriority;
   };
 
 protected:
@@ -129,9 +127,6 @@ class BasicThread:
   // Run condition:
   bool m_running = false;
 
-  // The current thread priority
-  ThreadPriority m_priority = ThreadPriority::Default;
-
   /// <summary>
   /// Assigns a name to the thread, displayed in debuggers.
   /// </summary>
@@ -150,7 +145,41 @@ class BasicThread:
   /// invoked before the thread starts to run, the thread will take on the specified priority when
   /// it is started.
   /// </remarks>
-  void SetThreadPriority(ThreadPriority threadPriority);
+  /// <returns>
+  /// The previous thread priority
+  /// </returns>
+  ThreadPriority SetThreadPriority(ThreadPriority threadPriority);
+
+  /// <summary>
+  /// Sets the thread priority of this thread only if it elevates the priority
+  /// </summary>
+  /// <remarks>
+  /// This method may be called while the thread is running, or before it starts to run.  If it is
+  /// invoked before the thread starts to run, the thread will take on the specified priority when
+  /// it is started.
+  /// </remarks>
+  /// <returns>
+  /// The previous thread priority
+  /// </returns>
+  ThreadPriority ElevateThreadPriority(ThreadPriority threadPriority);
+
+  /// <summary>
+  /// Sets the thread priority of this thread only if it de-elevates the priority
+  /// </summary>
+  /// <remarks>
+  /// This method may be called while the thread is running, or before it starts to run.  If it is
+  /// invoked before the thread starts to run, the thread will take on the specified priority when
+  /// it is started.
+  /// </remarks>
+  /// <returns>
+  /// The previous thread priority
+  /// </returns>
+  ThreadPriority DeelevateThreadPriority(ThreadPriority threadPriority);
+
+  /// <summary<>
+  /// Low-level function to set the thread priority
+  /// </summary>
+  static void SetThreadPriority(const std::thread::native_handle_type& handle, ThreadPriority threadPriority);
 
   /// <summary>
   /// Recovers a general lock used to synchronize entities in this thread internally.
@@ -217,7 +246,7 @@ class BasicThread:
   /// <returns>
   /// The current thread priority
   /// </returns>
-  ThreadPriority GetThreadPriority(void) const { return m_priority; }
+  ThreadPriority GetThreadPriority(void);
 
   /// <returns>
   /// True if this thread has transitioned to a completed state
@@ -310,6 +339,15 @@ class BasicThread:
   /// True if the calling thread is the main thread
   /// </returns>
   static bool IsMainThread(void);
+
+private:
+  /// <summary>
+  /// Update the thread priority to its current value
+  /// </summary>
+  /// <remarks>
+  /// Useful for re-setting the priority of a thread that may have be specified before it was started.
+  /// </remarks>
+  void UpdateThreadPriority(std::unique_lock<std::mutex>&& lock);
 };
 
 /// <summary>

src/autowiring/BasicThreadStateBlock.cpp

@@ -1,10 +1,12 @@
 // Copyright (C) 2012-2017 Leap Motion, Inc. All rights reserved.
 #include "stdafx.h"
 #include "BasicThreadStateBlock.h"
+#include "BasicThread.h"
 
 using namespace autowiring;
 
-BasicThreadStateBlock::BasicThreadStateBlock(void)
+BasicThreadStateBlock::BasicThreadStateBlock(void) :
+  m_priority{ ThreadPriority::Default }
 {}
 
 BasicThreadStateBlock::~BasicThreadStateBlock(void)

src/autowiring/BasicThreadStateBlock.h

@@ -4,6 +4,8 @@
 #include MUTEX_HEADER
 #include THREAD_HEADER
 
+enum class ThreadPriority;
+
 namespace autowiring {
 
 struct BasicThreadStateBlock:
@@ -12,13 +14,18 @@ struct BasicThreadStateBlock:
   BasicThreadStateBlock(void);
   ~BasicThreadStateBlock(void);
 
+  // Lock used to protect the actual thread
+  std::mutex m_threadLock;
+
   // General purpose thread lock and update condition for the lock
   std::mutex m_lock;
   std::condition_variable m_stateCondition;
 
   // The current thread, if running
   std::thread m_thisThread;
 
+  ThreadPriority m_priority;
+
   // Completion condition, true when this thread is no longer running and has run at least once
   bool m_completed = false;
 };

src/autowiring/CoreContextStateBlock.cpp

@@ -34,6 +34,7 @@ RunCounter::~RunCounter(void) {
     outstanding = std::move(stateBlock->m_outstanding);
     stateBlock->m_outstanding.reset();
   }
+  outstanding.reset();
 
   // Wake everyone up
   stateBlock->m_stateChanged.notify_all();

src/autowiring/CoreThread.cpp

@@ -15,7 +15,11 @@ void CoreThread::DoRunLoopCleanup(std::shared_ptr<CoreContext>&& ctxt, std::shar
   {
     CurrentContextPusher pshr(ctxt);
     // Only allow one thread at a time to clean up the dispatch queue
-    std::lock_guard<std::mutex> lk(m_stoppingLock);
+    std::unique_lock<std::mutex> lk(m_stoppingLock);
+    if (ShouldStop()) {
+      // If we are stopping, wait until this thread's OnStop function has completed
+      m_stoppingCond.wait(lk, [this] { return m_onStopCompleted; });
+    }
     Rundown();
   }
 
@@ -30,7 +34,6 @@ void CoreThread::Run() {
 
 void CoreThread::OnStop(bool graceful) {
   // Only allow one thread at a time to clean up the dispatch queue
-  std::lock_guard<std::mutex> lk(m_stoppingLock);
 
   // Base class handling first:
   BasicThread::OnStop(graceful);
@@ -46,4 +49,9 @@ void CoreThread::OnStop(bool graceful) {
   } else
     // Abort the dispatch queue so anyone waiting will wake up
     DispatchQueue::Abort();
+
+  // When OnStop has completed, then we may continue with DoRunLoopCleanup
+  std::lock_guard<std::mutex> lk(m_stoppingLock);
+  m_onStopCompleted = true;
+  m_stoppingCond.notify_all();
 }

src/autowiring/CoreThread.h

@@ -38,9 +38,11 @@ class CoreThread:
 
 protected:
   /// <summary>
-  /// While stopping, make sure we do it exclusively
+  /// While stopping, make sure we do it cleanly
   /// </summary>
   std::mutex m_stoppingLock;
+  std::condition_variable m_stoppingCond;
+  bool m_onStopCompleted = false;
 
   /// <summary>
   /// Overridden here so we can rundown the dispatch queue

src/autowiring/CoreThreadLinux.cpp

@@ -28,7 +28,7 @@ void BasicThread::GetThreadTimes(std::chrono::milliseconds& kernelTime, std::chr
   userTime = std::chrono::duration_cast<milliseconds>(seconds(usage.ru_utime.tv_sec) + microseconds(usage.ru_utime.tv_usec));
 }
 
-void BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
+void BasicThread::SetThreadPriority(const std::thread::native_handle_type& handle, ThreadPriority threadPriority) {
   struct sched_param param = { 0 };
   int policy = SCHED_OTHER;
   int percent = 0;
@@ -66,8 +66,7 @@ void BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
     throw std::invalid_argument("Attempted to assign an unrecognized thread priority");
   }
   min_priority = sched_get_priority_min(policy);
-  pthread_getschedparam(m_state->m_thisThread.native_handle(), &policy, &param);
+  pthread_getschedparam(handle, &policy, &param);
   param.sched_priority = min_priority + (percent * (sched_get_priority_max(policy) - min_priority) + 50) / 100;
-  pthread_setschedparam(m_state->m_thisThread.native_handle(), policy, &param);
-  m_priority = threadPriority;
+  pthread_setschedparam(handle, policy, &param);
 }

src/autowiring/CoreThreadMac.cpp

@@ -52,7 +52,7 @@ void BasicThread::GetThreadTimes(std::chrono::milliseconds& kernelTime, std::chr
   userTime = std::chrono::duration_cast<milliseconds>(nanoseconds(info.pth_user_time));
 }
 
-void BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
+void BasicThread::SetThreadPriority(const std::thread::native_handle_type& handle, ThreadPriority threadPriority) {
   struct sched_param param = { 0 };
   int policy = SCHED_OTHER;
   int percent = 0;
@@ -84,8 +84,7 @@ void BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
   default:
     throw std::invalid_argument("Attempted to assign an unrecognized thread priority");
   }
-  pthread_getschedparam(m_state->m_thisThread.native_handle(), &policy, &param);
+  pthread_getschedparam(handle, &policy, &param);
   param.sched_priority = PTHREAD_MIN_PRIORITY + (percent*(PTHREAD_MAX_PRIORITY - PTHREAD_MIN_PRIORITY) + 50) / 100;
-  pthread_setschedparam(m_state->m_thisThread.native_handle(), policy, &param);
-  m_priority = threadPriority;
+  pthread_setschedparam(handle, policy, &param);
 }

src/autowiring/CoreThreadWin.cpp

@@ -60,7 +60,7 @@ bool SetCapturePriority(void) {
   return true;
 }
 
-void BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
+void BasicThread::SetThreadPriority(const std::thread::native_handle_type& handle, ThreadPriority threadPriority) {
   int nPriority;
   switch(threadPriority) {
   case ThreadPriority::Idle:
@@ -98,10 +98,9 @@ void BasicThread::SetThreadPriority(ThreadPriority threadPriority) {
   }
 
   ::SetThreadPriority(
-    m_state->m_thisThread.native_handle(),
+    handle,
     nPriority
   );
-  m_priority = threadPriority;
 }
 
 std::chrono::steady_clock::time_point BasicThread::GetCreationTime(void) {