gst/gsttask.*: Detect and warn for obvious deadlocks. fixes #320340
authorWim Taymans <wim.taymans@gmail.com>
Mon, 13 Feb 2006 17:03:23 +0000 (17:03 +0000)
committerWim Taymans <wim.taymans@gmail.com>
Mon, 13 Feb 2006 17:03:23 +0000 (17:03 +0000)
Original commit message from CVS:
* gst/gsttask.c: (gst_task_init), (gst_task_func),
(gst_task_set_lock), (gst_task_start), (gst_task_pause),
(gst_task_join):
* gst/gsttask.h:
Detect and warn for obvious deadlocks. fixes #320340
Fix error case where lock was not released.

* tests/check/Makefile.am:
* tests/check/gst/gsttask.c: (task_func2), (GST_START_TEST),
(task_func), (gst_element_suite), (main):
Add task check.

ChangeLog
gst/gsttask.c
gst/gsttask.h
tests/check/Makefile.am
tests/check/gst/gsttask.c [new file with mode: 0644]

index ed36835..9201702 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2006-02-13  Wim Taymans  <wim@fluendo.com>
 
+       * gst/gsttask.c: (gst_task_init), (gst_task_func),
+       (gst_task_set_lock), (gst_task_start), (gst_task_pause),
+       (gst_task_join):
+       * gst/gsttask.h:
+       Detect and warn for obvious deadlocks. fixes #320340
+       Fix error case where lock was not released.
+
+       * tests/check/Makefile.am:
+       * tests/check/gst/gsttask.c: (task_func2), (GST_START_TEST),
+       (task_func), (gst_element_suite), (main):
+       Add task check.
+
+2006-02-13  Wim Taymans  <wim@fluendo.com>
+
        * docs/gst/gstreamer-sections.txt:
        * gst/gstbus.c:
        Add new functions to docs.
index 6393fa6..d6d01d5 100644 (file)
@@ -55,7 +55,7 @@
  * After creating a #GstTask, use gst_object_unref() to free its resources. This can
  * only be done it the task is not running anymore.
  *
- * Last reviewed on 2005-11-09 (0.9.4)
+ * Last reviewed on 2006-02-13 (0.10.4)
  */
 
 #include "gst_private.h"
@@ -122,6 +122,7 @@ static void
 gst_task_init (GstTask * task)
 {
   task->running = FALSE;
+  task->abidata.ABI.thread = NULL;
   task->lock = NULL;
   task->cond = g_cond_new ();
   task->state = GST_TASK_STOPPED;
@@ -146,8 +147,11 @@ static void
 gst_task_func (GstTask * task, GstTaskClass * tclass)
 {
   GStaticRecMutex *lock;
+  GThread *tself;
 
-  GST_DEBUG ("Entering task %p, thread %p", task, g_thread_self ());
+  tself = g_thread_self ();
+
+  GST_DEBUG ("Entering task %p, thread %p", task, tself);
 
   /* we have to grab the lock to get the mutex. We also
    * mark our state running so that nobody can mess with
@@ -156,7 +160,10 @@ gst_task_func (GstTask * task, GstTaskClass * tclass)
   if (task->state == GST_TASK_STOPPED)
     goto exit;
   lock = GST_TASK_GET_LOCK (task);
+  if (G_UNLIKELY (lock == NULL))
+    goto no_lock;
   task->running = TRUE;
+  task->abidata.ABI.thread = tself;
   GST_OBJECT_UNLOCK (task);
 
   /* locking order is TASK_LOCK, LOCK */
@@ -194,6 +201,7 @@ done:
   /* now we allow messing with the lock again */
   GST_OBJECT_LOCK (task);
   task->running = FALSE;
+  task->abidata.ABI.thread = NULL;
 exit:
   GST_TASK_SIGNAL (task);
   GST_OBJECT_UNLOCK (task);
@@ -201,6 +209,13 @@ exit:
   GST_DEBUG ("Exit task %p, thread %p", task, g_thread_self ());
 
   gst_object_unref (task);
+  return;
+
+no_lock:
+  {
+    g_warning ("starting task without a lock");
+    goto exit;
+  }
 }
 
 /**
@@ -267,6 +282,9 @@ gst_task_create (GstTaskFunction func, gpointer data)
  * Set the mutex used by the task. The mutex will be acquired before
  * calling the #GstTaskFunction.
  *
+ * This function has to be called before calling gst_task_pause() or
+ * gst_task_start().
+ *
  * MT safe.
  */
 void
@@ -283,8 +301,8 @@ gst_task_set_lock (GstTask * task, GStaticRecMutex * mutex)
   /* ERRORS */
 is_running:
   {
-    g_warning ("cannot call set_lock on a running task");
     GST_OBJECT_UNLOCK (task);
+    g_warning ("cannot call set_lock on a running task");
   }
 }
 
@@ -369,6 +387,8 @@ gst_task_start (GstTask * task)
   /* ERRORS */
 no_lock:
   {
+    GST_WARNING_OBJECT (task, "starting task without a lock");
+    GST_OBJECT_UNLOCK (task);
     g_warning ("starting task without a lock");
     return FALSE;
   }
@@ -438,6 +458,9 @@ gst_task_pause (GstTask * task)
   GST_DEBUG_OBJECT (task, "Pausing task %p", task);
 
   GST_OBJECT_LOCK (task);
+  if (G_UNLIKELY (GST_TASK_GET_LOCK (task) == NULL))
+    goto no_lock;
+
   old = task->state;
   task->state = GST_TASK_PAUSED;
   switch (old) {
@@ -461,6 +484,15 @@ gst_task_pause (GstTask * task)
   GST_OBJECT_UNLOCK (task);
 
   return TRUE;
+
+  /* ERRORS */
+no_lock:
+  {
+    GST_WARNING_OBJECT (task, "pausing task without a lock");
+    GST_OBJECT_UNLOCK (task);
+    g_warning ("pausing task without a lock");
+    return FALSE;
+  }
 }
 
 /**
@@ -482,11 +514,17 @@ gst_task_pause (GstTask * task)
 gboolean
 gst_task_join (GstTask * task)
 {
+  GThread *tself;
+
   g_return_val_if_fail (GST_IS_TASK (task), FALSE);
 
-  GST_DEBUG_OBJECT (task, "Joining task %p", task);
+  tself = g_thread_self ();
+
+  GST_DEBUG_OBJECT (task, "Joining task %p, thread %p", task, tself);
 
   GST_OBJECT_LOCK (task);
+  if (tself == task->abidata.ABI.thread)
+    goto joining_self;
   task->state = GST_TASK_STOPPED;
   GST_TASK_SIGNAL (task);
   while (task->running)
@@ -496,4 +534,13 @@ gst_task_join (GstTask * task)
   GST_DEBUG_OBJECT (task, "Joined task %p", task);
 
   return TRUE;
+
+  /* ERRORS */
+joining_self:
+  {
+    GST_WARNING_OBJECT (task, "trying to join task from its thread");
+    GST_OBJECT_UNLOCK (task);
+    g_warning ("trying to join task %p from its thread would deadlock", task);
+    return FALSE;
+  }
 }
index 9cc28ba..5c002bb 100644 (file)
@@ -133,7 +133,15 @@ struct _GstTask {
   gboolean        running;
 
   /*< private >*/
-  gpointer _gst_reserved[GST_PADDING];
+  union {
+    struct {
+      /* thread this task is currently running in */
+      GThread  *thread;
+    } ABI;
+    /* adding + 0 to mark ABI change to be undone later */
+    gpointer _gst_reserved[GST_PADDING + 0];
+  } abidata;
+
 };
 
 struct _GstTaskClass {
index cff2b25..f7b21eb 100644 (file)
@@ -54,6 +54,7 @@ check_PROGRAMS =                              \
        gst/gstsystemclock                      \
        gst/gststructure                        \
        gst/gsttag                              \
+       gst/gsttask                             \
        gst/gstutils                            \
        gst/gstvalue                            \
        elements/fakesink                       \
diff --git a/tests/check/gst/gsttask.c b/tests/check/gst/gsttask.c
new file mode 100644 (file)
index 0000000..b27af9f
--- /dev/null
@@ -0,0 +1,218 @@
+/* GStreamer
+ * Copyright (C) 2005 Thomas Vander Stichele <thomas at apestaart dot org>
+ *
+ * gstelement.c: Unit test for GstElement
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Library General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 02111-1307, USA.
+ */
+
+#include <gst/check/gstcheck.h>
+
+static GMutex *task_lock;
+static GCond *task_cond;
+
+static GStaticRecMutex task_mutex = G_STATIC_REC_MUTEX_INIT;
+
+static void
+task_func2 (void *data)
+{
+  gboolean ret;
+  GstTask *t = *((GstTask **) data);
+
+  g_mutex_lock (task_lock);
+  GST_DEBUG ("signal");
+  g_cond_signal (task_cond);
+  g_mutex_unlock (task_lock);
+
+  ASSERT_WARNING (ret = gst_task_join (t));
+  fail_unless (ret == FALSE);
+}
+
+GST_START_TEST (test_join)
+{
+  GstTask *t;
+  gboolean ret;
+
+  t = gst_task_create (task_func2, &t);
+  fail_if (t == NULL);
+
+  gst_task_set_lock (t, &task_mutex);
+
+  task_cond = g_cond_new ();
+  task_lock = g_mutex_new ();
+
+  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 ("joining");
+  ret = gst_task_join (t);
+  fail_unless (ret == TRUE);
+
+  gst_object_unref (t);
+}
+
+GST_END_TEST;
+
+static void
+task_func (void *data)
+{
+  g_mutex_lock (task_lock);
+  GST_DEBUG ("signal");
+  g_cond_signal (task_cond);
+  g_mutex_unlock (task_lock);
+}
+
+GST_START_TEST (test_lock_start)
+{
+  GstTask *t;
+  gboolean ret;
+
+  t = gst_task_create (task_func, NULL);
+  fail_if (t == NULL);
+
+  gst_task_set_lock (t, &task_mutex);
+
+  task_cond = g_cond_new ();
+  task_lock = g_mutex_new ();
+
+  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);
+
+  /* cannot set mutex now */
+  ASSERT_WARNING (gst_task_set_lock (t, &task_mutex));
+
+  GST_DEBUG ("joining");
+  ret = gst_task_join (t);
+  fail_unless (ret == TRUE);
+
+  gst_object_unref (t);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_lock)
+{
+  GstTask *t;
+  gboolean ret;
+
+  t = gst_task_create (task_func, NULL);
+  fail_if (t == NULL);
+
+  gst_task_set_lock (t, &task_mutex);
+
+  GST_DEBUG ("pause");
+  ret = gst_task_pause (t);
+  fail_unless (ret == TRUE);
+
+  g_usleep (1 * G_USEC_PER_SEC / 2);
+
+  GST_DEBUG ("joining");
+  ret = gst_task_join (t);
+  fail_unless (ret == TRUE);
+
+  g_usleep (1 * G_USEC_PER_SEC / 2);
+
+  gst_object_unref (t);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_no_lock)
+{
+  GstTask *t;
+  gboolean ret;
+
+  t = gst_task_create (task_func, NULL);
+  fail_if (t == NULL);
+
+  /* stop should be possible without lock */
+  gst_task_stop (t);
+
+  /* pause should give a warning */
+  ASSERT_WARNING (ret = gst_task_pause (t));
+  fail_unless (ret == FALSE);
+
+  /* start should give a warning */
+  ASSERT_WARNING (ret = gst_task_start (t));
+  fail_unless (ret == FALSE);
+
+  /* stop should be possible without lock */
+  gst_task_stop (t);
+
+  gst_object_unref (t);
+}
+
+GST_END_TEST;
+
+GST_START_TEST (test_create)
+{
+  GstTask *t;
+
+  t = gst_task_create (task_func, NULL);
+  fail_if (t == NULL);
+
+  gst_object_unref (t);
+}
+
+GST_END_TEST;
+
+
+Suite *
+gst_element_suite (void)
+{
+  Suite *s = suite_create ("GstTask");
+  TCase *tc_chain = tcase_create ("task tests");
+
+  suite_add_tcase (s, tc_chain);
+  tcase_add_test (tc_chain, test_create);
+  tcase_add_test (tc_chain, test_no_lock);
+  tcase_add_test (tc_chain, test_lock);
+  tcase_add_test (tc_chain, test_lock_start);
+  tcase_add_test (tc_chain, test_join);
+
+  return s;
+}
+
+int
+main (int argc, char **argv)
+{
+  int nf;
+
+  Suite *s = gst_element_suite ();
+  SRunner *sr = srunner_create (s);
+
+  gst_check_init (&argc, &argv);
+
+  srunner_run_all (sr, CK_NORMAL);
+  nf = srunner_ntests_failed (sr);
+  srunner_free (sr);
+
+  return nf;
+}