Add lock_guard to thread_pool::thread_count

I think there's a possible race when calling thread_pool::thread_count
from a worker thread, if the main thread changes the number of threads
at the same time.

When this code was initially written, we didn't worry about this,
because this was only accessed from the main thread.  This is no
longer the case, though.

This patch fixes any potential problem by adding a lock_guard to the
method.  This doesn't seem too harmful because this is not called very
much and because I doubt this lock is highly contended.

While doing this I noticed that thread_pool::do_post_task could access
m_sized_at_least_once and m_thread_count without holding the lock.
This patch changes this method as well.

Approved-by: Kevin Buettner <kevinb@redhat.com>

diff --git a/gdbsupport/thread-pool.cc b/gdbsupport/thread-pool.cc
index 6940773d58abb8a1dc8b13c384543ebd4c36b914..4dc72e566428321ad3c287b2c9c07de98967bb07 100644 (file)
--- a/gdbsupport/thread-pool.cc
+++ b/gdbsupport/thread-pool.cc
@@ -148,6 +148,17 @@ thread_pool::~thread_pool ()
      case -- see the comment by the definition of g_thread_pool.  */
 }
 
+size_t
+thread_pool::thread_count ()
+{
+#if CXX_STD_THREAD
+  std::lock_guard<std::mutex> guard (m_tasks_mutex);
+  return m_thread_count;
+#else
+  return 0;
+#endif
+}
+
 void
 thread_pool::set_thread_count (size_t num_threads)
 {
@@ -197,21 +208,33 @@ thread_pool::set_thread_count (size_t num_threads)
 void
 thread_pool::do_post_task (std::packaged_task<void ()> &&func)
 {
-  /* This assert is here to check that no tasks are posted to the pool between
-     its initialization and sizing.  */
-  gdb_assert (m_sized_at_least_once);
-  std::packaged_task<void ()> t (std::move (func));
+  /* This is a bit strange but we don't want to hold the lock when
+     calling FUNC in the case where it must be called immediately.
+     Although that seems pathological, there are some weird cases (a
+     moribund worker thread or FUNC itself submitting a task) and it
+     seemed best to be safe.  */
+  bool call_immediately = false;
 
-  if (m_thread_count != 0)
-    {
-      std::lock_guard<std::mutex> guard (m_tasks_mutex);
-      m_tasks.emplace (std::move (t));
-      m_tasks_cv.notify_one ();
-    }
-  else
+  {
+    std::lock_guard<std::mutex> guard (m_tasks_mutex);
+
+    /* This assert is here to check that no tasks are posted to the
+       pool between its initialization and sizing.  */
+    gdb_assert (m_sized_at_least_once);
+
+    if (m_thread_count != 0)
+      {
+       m_tasks.emplace (std::move (func));
+       m_tasks_cv.notify_one ();
+      }
+    else
+      call_immediately = true;
+  }
+
+  if (call_immediately)
     {
       /* Just execute it now.  */
-      t ();
+      func ();
     }
 }
 

diff --git a/gdbsupport/thread-pool.h b/gdbsupport/thread-pool.h
index 50b7a064d7eaa66344353154b5b8fe16c34e6d17..a9188d1e10737ec321bc12158d14d8d6a4ea964e 100644 (file)
--- a/gdbsupport/thread-pool.h
+++ b/gdbsupport/thread-pool.h
@@ -50,14 +50,7 @@ public:
   void set_thread_count (size_t num_threads);
 
   /* Return the number of executing threads.  */
-  size_t thread_count () const
-  {
-#if CXX_STD_THREAD
-    return m_thread_count;
-#else
-    return 0;
-#endif
-  }
+  size_t thread_count ();
 
   /* Post a task to the thread pool.  A future is returned, which can
      be used to wait for the result.  */