task: Fix pause/stop race condition

If a task thread is calling pause on it self and the
controlling/"main" thread stops the task, it could end in a race
where gst_task_func loops and then checks for paused after the
controlling thread just changed the task state to stopped.
Hence the task would actually call func again even though it was
both paused and stopped.

https://bugzilla.gnome.org/show_bug.cgi?id=740001

diff --git a/gst/gsttask.c b/gst/gsttask.c
index b562f6c..60c0140 100644 (file)
--- a/gst/gsttask.c
+++ b/gst/gsttask.c
@@ -292,31 +292,30 @@ gst_task_func (GstTask * task)
   gst_task_configure_name (task);
 
   while (G_LIKELY (GET_TASK_STATE (task) != GST_TASK_STOPPED)) {
-    if (G_UNLIKELY (GET_TASK_STATE (task) == GST_TASK_PAUSED)) {
+    GST_OBJECT_LOCK (task);
+    while (G_UNLIKELY (GST_TASK_STATE (task) == GST_TASK_PAUSED)) {
+      g_rec_mutex_unlock (lock);
+
+      GST_TASK_SIGNAL (task);
+      GST_INFO_OBJECT (task, "Task going to paused");
+      GST_TASK_WAIT (task);
+      GST_INFO_OBJECT (task, "Task resume from paused");
+      GST_OBJECT_UNLOCK (task);
+      /* locking order.. */
+      g_rec_mutex_lock (lock);
       GST_OBJECT_LOCK (task);
-      while (G_UNLIKELY (GST_TASK_STATE (task) == GST_TASK_PAUSED)) {
-        g_rec_mutex_unlock (lock);
+    }
 
-        GST_TASK_SIGNAL (task);
-        GST_INFO_OBJECT (task, "Task going to paused");
-        GST_TASK_WAIT (task);
-        GST_INFO_OBJECT (task, "Task resume from paused");
-        GST_OBJECT_UNLOCK (task);
-        /* locking order.. */
-        g_rec_mutex_lock (lock);
-
-        GST_OBJECT_LOCK (task);
-        if (G_UNLIKELY (GET_TASK_STATE (task) == GST_TASK_STOPPED)) {
-          GST_OBJECT_UNLOCK (task);
-          goto done;
-        }
-      }
+    if (G_UNLIKELY (GET_TASK_STATE (task) == GST_TASK_STOPPED)) {
+      GST_OBJECT_UNLOCK (task);
+      break;
+    } else {
       GST_OBJECT_UNLOCK (task);
     }
 
     task->func (task->user_data);
   }
-done:
+
   g_rec_mutex_unlock (lock);
 
   GST_OBJECT_LOCK (task);

diff --git a/tests/check/gst/gsttask.c b/tests/check/gst/gsttask.c
index 986c403..db79558 100644 (file)
--- a/tests/check/gst/gsttask.c
+++ b/tests/check/gst/gsttask.c
@@ -26,6 +26,64 @@ static GCond task_cond;
 
 static GRecMutex task_mutex;
 
+#define TEST_RACE_ITERATIONS 1000
+
+static void
+task_signal_pause_func (void *data)
+{
+  GstTask **t = data;
+
+  g_mutex_lock (&task_lock);
+  GST_DEBUG ("signal");
+  g_cond_signal (&task_cond);
+
+  gst_task_pause (*t);
+  g_mutex_unlock (&task_lock);
+}
+
+GST_START_TEST (test_pause_stop_race)
+{
+  guint it = TEST_RACE_ITERATIONS;
+  GstTask *t;
+  gboolean ret;
+
+  t = gst_task_new (task_signal_pause_func, &t, NULL);
+  fail_if (t == NULL);
+
+  g_rec_mutex_init (&task_mutex);
+  gst_task_set_lock (t, &task_mutex);
+
+  g_cond_init (&task_cond);
+  g_mutex_init (&task_lock);
+
+  while (it-- > 0) {
+    g_mutex_lock (&task_lock);
+    GST_DEBUG ("starting");
+    ret = gst_task_start (t);
+    fail_unless (ret == TRUE);
+    /* wait for it to spin up */
+    GST_DEBUG ("waiting");
+    g_cond_wait (&task_cond, &task_lock);
+    GST_DEBUG ("done waiting");
+    g_mutex_unlock (&task_lock);
+
+    GST_DEBUG ("starting");
+    ret = gst_task_stop (t);
+    fail_unless (ret == TRUE);
+
+    GST_DEBUG ("joining");
+    ret = gst_task_join (t);
+    fail_unless (ret == TRUE);
+  }
+
+  g_cond_clear (&task_cond);
+  g_mutex_clear (&task_lock);
+
+  gst_object_unref (t);
+}
+
+GST_END_TEST;
+
 static void
 task_func2 (void *data)
 {
@@ -203,6 +261,7 @@ gst_task_suite (void)
   tcase_add_test (tc_chain, test_lock);
   tcase_add_test (tc_chain, test_lock_start);
   tcase_add_test (tc_chain, test_join);
+  tcase_add_test (tc_chain, test_pause_stop_race);
 
   return s;
 }