* dbus/dbus-threads.c: Allow recursive mutex's to be passed into

  dbus_threads_init and be used by the dbus mutex functions to
  avoid deadlocks.

* doc/TODO: Remove recursive mutex dbus_connection_dispatch TODO item

7 files changed, 109 insertions, 50 deletions

diff --git a/ChangeLog b/ChangeLog
index ae2e5c4a..beb6d264 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2006-09-14  John (J5) Palmieri  <johnp@redhat.com>
+
+	* dbus/dbus-threads.c: Allow recursive mutex's to be passed into
+	dbus_threads_init and be used by the dbus mutex functions to
+	avoid deadlocks.
+
+	* doc/TODO: Remove recursive mutex dbus_connection_dispatch TODO item
+
 2006-09-13  John (J5) Palmieri  <johnp@redhat.com>
 
 	* dbus/dbus-sysdeps-util-unix.c (_dbus_directory_get_next_file):
diff --git a/dbus/dbus-dataslot.c b/dbus/dbus-dataslot.c
index d53f3bd7..f9c12f84 100644
--- a/dbus/dbus-dataslot.c
+++ b/dbus/dbus-dataslot.c
@@ -70,8 +70,7 @@ _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator,
 {
   dbus_int32_t slot;
 
-  if (!_dbus_mutex_lock (*mutex_loc))
-    return FALSE;
+  _dbus_mutex_lock (*mutex_loc);
 
   if (allocator->n_allocated_slots == 0)
     {
@@ -246,8 +245,7 @@ _dbus_data_slot_list_set  (DBusDataSlotAllocator *allocator,
    * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts
    * are disabled, since then the asserts are empty.
    */
-  if (!_dbus_mutex_lock (*(allocator->lock_loc)))
-    return FALSE;
+  _dbus_mutex_lock (*(allocator->lock_loc));
   _dbus_assert (slot < allocator->n_allocated_slots);
   _dbus_assert (allocator->allocated_slots[slot].slot_id == slot);
   _dbus_mutex_unlock (*(allocator->lock_loc));
@@ -304,8 +302,7 @@ _dbus_data_slot_list_get  (DBusDataSlotAllocator *allocator,
    * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts
    * are disabled, since then the asserts are empty.
    */
-  if (!_dbus_mutex_lock (*(allocator->lock_loc)))
-    return NULL;
+  _dbus_mutex_lock (*(allocator->lock_loc));
   _dbus_assert (slot >= 0);
   _dbus_assert (slot < allocator->n_allocated_slots);
   _dbus_assert (allocator->allocated_slots[slot].slot_id == slot);
diff --git a/dbus/dbus-list.c b/dbus/dbus-list.c
index 394db5f5..09ece641 100644
--- a/dbus/dbus-list.c
+++ b/dbus/dbus-list.c
@@ -55,8 +55,7 @@ alloc_link (void *data)
 {
   DBusList *link;
 
-  if (!_DBUS_LOCK (list))
-    return NULL;
+  _DBUS_LOCK (list);
 
   if (list_pool == NULL)
     {      
diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h
index 700c4f75..8f26bca1 100644
--- a/dbus/dbus-threads-internal.h
+++ b/dbus/dbus-threads-internal.h
@@ -31,8 +31,8 @@ DBUS_BEGIN_DECLS
 
 DBusMutex*   _dbus_mutex_new                 (void);
 void         _dbus_mutex_free                (DBusMutex         *mutex);
-dbus_bool_t  _dbus_mutex_lock                (DBusMutex         *mutex);
-dbus_bool_t  _dbus_mutex_unlock              (DBusMutex         *mutex);
+void         _dbus_mutex_lock                (DBusMutex         *mutex);
+void         _dbus_mutex_unlock              (DBusMutex         *mutex);
 void         _dbus_mutex_new_at_location     (DBusMutex        **location_p);
 void         _dbus_mutex_free_at_location    (DBusMutex        **location_p);
 
diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c
index 251dd454..8c7eb5ef 100644
--- a/dbus/dbus-threads.c
+++ b/dbus/dbus-threads.c
@@ -39,10 +39,10 @@
 static DBusThreadFunctions thread_functions =
 {
   0,
-  NULL, NULL, NULL, NULL,
   NULL, NULL, NULL, NULL, NULL,
-
+  NULL, NULL, NULL, NULL, NULL,
   NULL, NULL, NULL, NULL,
+  
   NULL, NULL, NULL, NULL
 };
 
@@ -87,7 +87,9 @@ static DBusList *uninitialized_condvar_list = NULL;
 DBusMutex*
 _dbus_mutex_new (void)
 {
-  if (thread_functions.mutex_new)
+  if (thread_functions.recursive_mutex_new)
+    return (* thread_functions.recursive_mutex_new) ();
+  else if (thread_functions.mutex_new)
     return (* thread_functions.mutex_new) ();
   else
     return _DBUS_DUMMY_MUTEX;
@@ -126,8 +128,13 @@ _dbus_mutex_new_at_location (DBusMutex **location_p)
 void
 _dbus_mutex_free (DBusMutex *mutex)
 {
-  if (mutex && thread_functions.mutex_free)
-    (* thread_functions.mutex_free) (mutex);
+  if (mutex)
+    {
+      if (mutex && thread_functions.recursive_mutex_free)
+        (* thread_functions.recursive_mutex_free) (mutex);
+      else if (mutex && thread_functions.mutex_free)
+        (* thread_functions.mutex_free) (mutex);
+    }
 }
 
 /**
@@ -149,17 +156,19 @@ _dbus_mutex_free_at_location (DBusMutex **location_p)
 
 /**
  * Locks a mutex. Does nothing if passed a #NULL pointer.
- * Locks are not recursive.
- *
- * @returns #TRUE on success
+ * Locks may be recursive if threading implementation initialized
+ * recursive locks.
  */
-dbus_bool_t
+void
 _dbus_mutex_lock (DBusMutex *mutex)
 {
-  if (mutex && thread_functions.mutex_lock)
-    return (* thread_functions.mutex_lock) (mutex);
-  else
-    return TRUE;
+  if (mutex) 
+    {
+      if (thread_functions.recursive_mutex_lock)
+        (* thread_functions.recursive_mutex_lock) (mutex);
+      else if (thread_functions.mutex_lock)
+        (* thread_functions.mutex_lock) (mutex);
+    }
 }
 
 /**
@@ -167,13 +176,16 @@ _dbus_mutex_lock (DBusMutex *mutex)
  *
  * @returns #TRUE on success
  */
-dbus_bool_t
+void
 _dbus_mutex_unlock (DBusMutex *mutex)
 {
-  if (mutex && thread_functions.mutex_unlock)
-    return (* thread_functions.mutex_unlock) (mutex);
-  else
-    return TRUE;
+  if (mutex)
+    {
+      if (thread_functions.recursive_mutex_unlock)
+        (* thread_functions.recursive_mutex_unlock) (mutex);
+      else if (thread_functions.mutex_unlock)
+        (* thread_functions.mutex_unlock) (mutex);
+    }
 }
 
 /**
@@ -515,25 +527,20 @@ init_locks (void)
 dbus_bool_t
 dbus_threads_init (const DBusThreadFunctions *functions)
 {
+  dbus_bool_t mutex_set;
+  dbus_bool_t recursive_mutex_set;
+
   _dbus_assert (functions != NULL);
 
   /* these base functions are required. Future additions to
    * DBusThreadFunctions may be optional.
    */
-  _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_NEW_MASK);
-  _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_FREE_MASK);
-  _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_LOCK_MASK);
-  _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_UNLOCK_MASK);
   _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_NEW_MASK);
   _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_FREE_MASK);
   _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAIT_MASK);
   _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAIT_TIMEOUT_MASK);
   _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ONE_MASK);
   _dbus_assert (functions->mask & DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ALL_MASK);
-  _dbus_assert (functions->mutex_new != NULL);
-  _dbus_assert (functions->mutex_free != NULL);
-  _dbus_assert (functions->mutex_lock != NULL);
-  _dbus_assert (functions->mutex_unlock != NULL);
   _dbus_assert (functions->condvar_new != NULL);
   _dbus_assert (functions->condvar_free != NULL);
   _dbus_assert (functions->condvar_wait != NULL);
@@ -541,6 +548,40 @@ dbus_threads_init (const DBusThreadFunctions *functions)
   _dbus_assert (functions->condvar_wake_one != NULL);
   _dbus_assert (functions->condvar_wake_all != NULL);
 
+  /* Either the mutex function set or recursive mutex set needs 
+   * to be available but not both
+   */
+  mutex_set = (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_NEW_MASK) &&  
+              (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_FREE_MASK) && 
+              (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_LOCK_MASK) &&
+              (functions->mask & DBUS_THREAD_FUNCTIONS_MUTEX_UNLOCK_MASK) &&
+               functions->mutex_new &&
+               functions->mutex_free &&
+               functions->mutex_lock &&
+               functions->mutex_unlock;
+
+  recursive_mutex_set = 
+              (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_NEW_MASK) && 
+              (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_FREE_MASK) && 
+              (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_LOCK_MASK) && 
+              (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_UNLOCK_MASK) &&
+                functions->recursive_mutex_new &&
+                functions->recursive_mutex_free &&
+                functions->recursive_mutex_lock &&
+                functions->recursive_mutex_unlock;
+
+  if (!(mutex_set || recursive_mutex_set))
+    _dbus_assert_not_reached ("Either the nonrecusrive or recursive mutex " 
+                              "functions sets should be passed into "
+                              "dbus_threads_init. Neither sets were passed.");
+
+  if (mutex_set && recursive_mutex_set)
+    _dbus_assert_not_reached ("Either the nonrecusrive or recursive mutex " 
+                              "functions sets should be passed into "
+                              "dbus_threads_init. Both sets were passed. "
+                              "You most likely just want to set the recursive "
+                              "mutex functions to avoid deadlocks in D-Bus.");
+                          
   /* Check that all bits in the mask actually are valid mask bits.
    * ensures people won't write code that breaks when we add
    * new bits.
@@ -567,7 +608,19 @@ dbus_threads_init (const DBusThreadFunctions *functions)
   thread_functions.condvar_wait_timeout = functions->condvar_wait_timeout;
   thread_functions.condvar_wake_one = functions->condvar_wake_one;
   thread_functions.condvar_wake_all = functions->condvar_wake_all;
+ 
+  if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_NEW_MASK)
+    thread_functions.recursive_mutex_new = functions->recursive_mutex_new;
   
+  if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_FREE_MASK)
+    thread_functions.recursive_mutex_free = functions->recursive_mutex_free;
+  
+  if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_LOCK_MASK)
+    thread_functions.recursive_mutex_lock = functions->recursive_mutex_lock;
+
+  if (functions->mask & DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_UNLOCK_MASK)
+    thread_functions.recursive_mutex_unlock = functions->recursive_mutex_unlock;
+
   thread_functions.mask = functions->mask;
 
   if (!init_locks ())
diff --git a/dbus/dbus-threads.h b/dbus/dbus-threads.h
index 8d8e072a..5e5a67ef 100644
--- a/dbus/dbus-threads.h
+++ b/dbus/dbus-threads.h
@@ -40,6 +40,11 @@ typedef void        (* DBusMutexFreeFunction)   (DBusMutex *mutex);
 typedef dbus_bool_t (* DBusMutexLockFunction)   (DBusMutex *mutex);
 typedef dbus_bool_t (* DBusMutexUnlockFunction) (DBusMutex *mutex);
 
+typedef DBusMutex*  (* DBusRecursiveMutexNewFunction)    (void);
+typedef void        (* DBusRecursiveMutexFreeFunction)   (DBusMutex *mutex);
+typedef void        (* DBusRecursiveMutexLockFunction)   (DBusMutex *mutex);
+typedef void        (* DBusRecursiveMutexUnlockFunction) (DBusMutex *mutex);
+
 typedef DBusCondVar*  (* DBusCondVarNewFunction)         (void);
 typedef void          (* DBusCondVarFreeFunction)        (DBusCondVar *cond);
 typedef void          (* DBusCondVarWaitFunction)        (DBusCondVar *cond,
@@ -62,8 +67,11 @@ typedef enum
   DBUS_THREAD_FUNCTIONS_CONDVAR_WAIT_TIMEOUT_MASK   = 1 << 7,
   DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ONE_MASK = 1 << 8,
   DBUS_THREAD_FUNCTIONS_CONDVAR_WAKE_ALL_MASK = 1 << 9,
-
-  DBUS_THREAD_FUNCTIONS_ALL_MASK     = (1 << 10) - 1
+  DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_NEW_MASK    = 1 << 10,
+  DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_FREE_MASK   = 1 << 11,
+  DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_LOCK_MASK   = 1 << 12,
+  DBUS_THREAD_FUNCTIONS_RECURSIVE_MUTEX_UNLOCK_MASK = 1 << 13,
+  DBUS_THREAD_FUNCTIONS_ALL_MASK     = (1 << 13) - 1
 } DBusThreadFunctionsMask;
 
 /**
@@ -85,15 +93,16 @@ typedef struct
   DBusCondVarWaitTimeoutFunction condvar_wait_timeout; /**< Function to wait on a condition with a timeout */
   DBusCondVarWakeOneFunction condvar_wake_one; /**< Function to wake one thread waiting on the condition */
   DBusCondVarWakeAllFunction condvar_wake_all; /**< Function to wake all threads waiting on the condition */
-  
+ 
+  DBusRecursiveMutexNewFunction recursive_mutex_new; /**< Function to create a recursive mutex */
+  DBusRecursiveMutexFreeFunction recursive_mutex_free; /**< Function to free a recursive mutex */
+  DBusRecursiveMutexLockFunction recursive_mutex_lock; /**< Function to lock a recursive mutex */
+  DBusRecursiveMutexUnlockFunction recursive_mutex_unlock; /**< Function to unlock a recursive mutex */
+
   void (* padding1) (void); /**< Reserved for future expansion */
   void (* padding2) (void); /**< Reserved for future expansion */
   void (* padding3) (void); /**< Reserved for future expansion */
   void (* padding4) (void); /**< Reserved for future expansion */
-  void (* padding5) (void); /**< Reserved for future expansion */
-  void (* padding6) (void); /**< Reserved for future expansion */
-  void (* padding7) (void); /**< Reserved for future expansion */
-  void (* padding8) (void); /**< Reserved for future expansion */
   
 } DBusThreadFunctions;
 
diff --git a/doc/TODO b/doc/TODO
index d3ece326..fc9ef2e3 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -1,13 +1,6 @@
 Important for 1.0
 ===
 
- - add a new return code from dbus_connection_dispatch() called 
-   IN_PROGRESS or RECURSED or something, indicating that DATA_REMAINS
-   but another dispatch is in progress, so we can't dispatch at 
-   this time. OR maybe just switch to recursive locks for the dispatch 
-   locks. Fixes the recursive deadlock. See the @todo for more
-   and this thread: http://lists.freedesktop.org/archives/dbus/2006-February/004128.html
-
  - Take a look at the issues marked @todo 1.0 or FIXME 1.0.  Ones with
    Question marks at the ends either need clarification or are not 
    really needed for 1.0 but would be nice.