From 2681eaea50e6c8cc1fa5511359202b83cb3e1412 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Mon, 13 Feb 2006 17:03:23 +0000 Subject: [PATCH] gst/gsttask.*: Detect and warn for obvious deadlocks. fixes #320340 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 | 14 +++ gst/gsttask.c | 55 +++++++++++- gst/gsttask.h | 10 ++- tests/check/Makefile.am | 1 + tests/check/gst/gsttask.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 293 insertions(+), 5 deletions(-) create mode 100644 tests/check/gst/gsttask.c diff --git a/ChangeLog b/ChangeLog index ed36835..9201702 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,19 @@ 2006-02-13 Wim Taymans + * 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 + * docs/gst/gstreamer-sections.txt: * gst/gstbus.c: Add new functions to docs. diff --git a/gst/gsttask.c b/gst/gsttask.c index 6393fa6..d6d01d5 100644 --- a/gst/gsttask.c +++ b/gst/gsttask.c @@ -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; + } } diff --git a/gst/gsttask.h b/gst/gsttask.h index 9cc28ba..5c002bb 100644 --- a/gst/gsttask.h +++ b/gst/gsttask.h @@ -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 { diff --git a/tests/check/Makefile.am b/tests/check/Makefile.am index cff2b25..f7b21eb 100644 --- a/tests/check/Makefile.am +++ b/tests/check/Makefile.am @@ -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 index 0000000..b27af9f --- /dev/null +++ b/tests/check/gst/gsttask.c @@ -0,0 +1,218 @@ +/* GStreamer + * Copyright (C) 2005 Thomas Vander Stichele + * + * 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 + +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; +} -- 2.7.4