path: root/sys-apps/dbus/files/multithread-locking-fix.patch

2008-01-01  Havoc Pennington  <hp at redhat.com>

	Patch based on one from Olivier Hochreutiner <olivier.hochreutiner
	gmail.com>

	* dbus/dbus-connection.c (protected_change_timeout): remove the
	elaborate nonworking hack to try to drop locks and just keep the
	locks; this isn't right either, but at least is correct, though
	it puts restrictions on apps.

	* dbus/dbus-connection.c (protected_change_watch): make the same
	change as for timeouts

	* dbus/dbus-connection.c (dbus_connection_set_timeout_functions):
	don't drop the lock here; add documentation of the problem to API
	docs
	(dbus_connection_set_watch_functions): same

	* dbus/dbus-connection.c (dbus_connection_get_data)
	(dbus_connection_set_data): introduce a separate slot_mutex
	protecting connection->slot_list so these two functions can be
	called inside watch and timeout functions. Not sure this
	is going to be a good idea.

	* dbus/dbus-connection.c (dbus_connection_unref)
	(dbus_connection_ref): avoid using connection lock in ref/unref
	so these can also be used in watch and timeout functions
---
 ChangeLog              |   28 +++++++
 dbus/dbus-connection.c |  196 ++++++++++++++++++++++++++----------------------
 2 files changed, 134 insertions(+), 90 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 35e35a7..ec761f3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,31 @@
+2008-01-01  Havoc Pennington  <hp at redhat.com>
+
+	Patch based on one from Olivier Hochreutiner <olivier.hochreutiner
+	gmail.com>
+	
+	* dbus/dbus-connection.c (protected_change_timeout): remove the
+	elaborate nonworking hack to try to drop locks and just keep the
+	locks; this isn't right either, but at least is correct, though
+	it puts restrictions on apps.
+
+	* dbus/dbus-connection.c (protected_change_watch): make the same
+	change as for timeouts	
+
+	* dbus/dbus-connection.c (dbus_connection_set_timeout_functions):
+	don't drop the lock here; add documentation of the problem to API
+	docs
+	(dbus_connection_set_watch_functions): same
+
+	* dbus/dbus-connection.c (dbus_connection_get_data)
+	(dbus_connection_set_data): introduce a separate slot_mutex
+	protecting connection->slot_list so these two functions can be
+	called inside watch and timeout functions. Not sure this 
+	is going to be a good idea.
+
+	* dbus/dbus-connection.c (dbus_connection_unref)
+	(dbus_connection_ref): avoid using connection lock in ref/unref 
+	so these can also be used in watch and timeout functions
+	
 2008-04-16  John (J5) Palmieri <johnp@redhat.com>
 
 	* tools/dbus-monitor.c: take out the trailing comma in the
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 0de5f22..177fb2e 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -73,6 +73,14 @@
     _dbus_mutex_unlock ((connection)->mutex);                                            \
   } while (0)
 
+#define SLOTS_LOCK(connection) do {                     \
+    _dbus_mutex_lock ((connection)->slot_mutex);        \
+  } while (0)
+
+#define SLOTS_UNLOCK(connection) do {                   \
+    _dbus_mutex_unlock ((connection)->slot_mutex);      \
+  } while (0)
+
 #define DISPATCH_STATUS_NAME(s)                                            \
                      ((s) == DBUS_DISPATCH_COMPLETE ? "complete" :         \
                       (s) == DBUS_DISPATCH_DATA_REMAINS ? "data remains" : \
@@ -257,6 +265,7 @@ struct DBusConnection
   
   DBusList *filter_list;        /**< List of filters. */
 
+  DBusMutex *slot_mutex;        /**< Lock on slot_list so overall connection lock need not be taken */
   DBusDataSlotList slot_list;   /**< Data stored by allocated integer ID */
 
   DBusHashTable *pending_replies;  /**< Hash of message serials to #DBusPendingCall. */  
@@ -647,38 +656,42 @@ protected_change_watch (DBusConnection         *connection,
                         DBusWatchToggleFunction toggle_function,
                         dbus_bool_t             enabled)
 {
-  DBusWatchList *watches;
   dbus_bool_t retval;
   
   HAVE_LOCK_CHECK (connection);
 
-  /* This isn't really safe or reasonable; a better pattern is the "do everything, then
-   * drop lock and call out" one; but it has to be propagated up through all callers
+  /* The original purpose of protected_change_watch() was to hold a
+   * ref on the connection while dropping the connection lock, then
+   * calling out to the app.  This was a broken hack that did not
+   * work, since the connection was in a hosed state (no WatchList
+   * field) while calling out.
+   *
+   * So for now we'll just keep the lock while calling out. This means
+   * apps are not allowed to call DBusConnection methods inside a
+   * watch function or they will deadlock.
+   * 
+   * The "real fix" is to use the _and_unlock() pattern found
+   * elsewhere in the code, to defer calling out to the app until
+   * we're about to drop locks and return flow of control to the app
+   * anyway.
+   *
+   * See http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
    */
   
-  watches = connection->watches;
-  if (watches)
+  if (connection->watches)
     {
-      connection->watches = NULL;
-      _dbus_connection_ref_unlocked (connection);
-      CONNECTION_UNLOCK (connection);
-
       if (add_function)
-        retval = (* add_function) (watches, watch);
+        retval = (* add_function) (connection->watches, watch);
       else if (remove_function)
         {
           retval = TRUE;
-          (* remove_function) (watches, watch);
+          (* remove_function) (connection->watches, watch);
         }
       else
         {
           retval = TRUE;
-          (* toggle_function) (watches, watch, enabled);
+          (* toggle_function) (connection->watches, watch, enabled);
         }
-      
-      CONNECTION_LOCK (connection);
-      connection->watches = watches;
-      _dbus_connection_unref_unlocked (connection);
 
       return retval;
     }
@@ -768,39 +781,43 @@ protected_change_timeout (DBusConnection           *connection,
                           DBusTimeoutToggleFunction toggle_function,
                           dbus_bool_t               enabled)
 {
-  DBusTimeoutList *timeouts;
   dbus_bool_t retval;
   
   HAVE_LOCK_CHECK (connection);
 
-  /* This isn't really safe or reasonable; a better pattern is the "do everything, then
-   * drop lock and call out" one; but it has to be propagated up through all callers
+  /* The original purpose of protected_change_timeout() was to hold a
+   * ref on the connection while dropping the connection lock, then
+   * calling out to the app.  This was a broken hack that did not
+   * work, since the connection was in a hosed state (no TimeoutList
+   * field) while calling out.
+   *
+   * So for now we'll just keep the lock while calling out. This means
+   * apps are not allowed to call DBusConnection methods inside a
+   * timeout function or they will deadlock.
+   * 
+   * The "real fix" is to use the _and_unlock() pattern found
+   * elsewhere in the code, to defer calling out to the app until
+   * we're about to drop locks and return flow of control to the app
+   * anyway.
+   *
+   * See http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
    */
   
-  timeouts = connection->timeouts;
-  if (timeouts)
+  if (connection->timeouts)
     {
-      connection->timeouts = NULL;
-      _dbus_connection_ref_unlocked (connection);
-      CONNECTION_UNLOCK (connection);
-
       if (add_function)
-        retval = (* add_function) (timeouts, timeout);
+        retval = (* add_function) (connection->timeouts, timeout);
       else if (remove_function)
         {
           retval = TRUE;
-          (* remove_function) (timeouts, timeout);
+          (* remove_function) (connection->timeouts, timeout);
         }
       else
         {
           retval = TRUE;
-          (* toggle_function) (timeouts, timeout, enabled);
+          (* toggle_function) (connection->timeouts, timeout, enabled);
         }
       
-      CONNECTION_LOCK (connection);
-      connection->timeouts = timeouts;
-      _dbus_connection_unref_unlocked (connection);
-
       return retval;
     }
   else
@@ -1219,6 +1236,10 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
   if (connection->io_path_cond == NULL)
     goto error;
 
+  _dbus_mutex_new_at_location (&connection->slot_mutex);
+  if (connection->slot_mutex == NULL)
+    goto error;
+  
   disconnect_message = dbus_message_new_signal (DBUS_PATH_LOCAL,
                                                 DBUS_INTERFACE_LOCAL,
                                                 "Disconnected");
@@ -1295,6 +1316,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
       _dbus_mutex_free_at_location (&connection->mutex);
       _dbus_mutex_free_at_location (&connection->io_path_mutex);
       _dbus_mutex_free_at_location (&connection->dispatch_mutex);
+      _dbus_mutex_free_at_location (&connection->slot_mutex);
       dbus_free (connection);
     }
   if (pending_replies)
@@ -2467,9 +2489,15 @@ dbus_connection_ref (DBusConnection *connection)
   
   /* The connection lock is better than the global
    * lock in the atomic increment fallback
+   *
+   * (FIXME but for now we always use the atomic version,
+   * to avoid taking the connection lock, due to
+   * the mess with set_timeout_functions()/set_watch_functions()
+   * calling out to the app without dropping locks)
    */
   
-#ifdef DBUS_HAVE_ATOMIC_INT
+  /* #ifdef DBUS_HAVE_ATOMIC_INT */
+#if 1
   _dbus_atomic_inc (&connection->refcount);
 #else
   CONNECTION_LOCK (connection);
@@ -2581,6 +2609,8 @@ _dbus_connection_last_unref (DBusConnection *connection)
   _dbus_mutex_free_at_location (&connection->io_path_mutex);
   _dbus_mutex_free_at_location (&connection->dispatch_mutex);
 
+  _dbus_mutex_free_at_location (&connection->slot_mutex);
+  
   _dbus_mutex_free_at_location (&connection->mutex);
   
   dbus_free (connection);
@@ -2615,9 +2645,15 @@ dbus_connection_unref (DBusConnection *connection)
   
   /* The connection lock is better than the global
    * lock in the atomic increment fallback
+   * 
+   * (FIXME but for now we always use the atomic version,
+   * to avoid taking the connection lock, due to
+   * the mess with set_timeout_functions()/set_watch_functions()
+   * calling out to the app without dropping locks)   
    */
   
-#ifdef DBUS_HAVE_ATOMIC_INT
+  /* #ifdef DBUS_HAVE_ATOMIC_INT */
+#if 1
   last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
 #else
   CONNECTION_LOCK (connection);
@@ -4550,10 +4586,12 @@ dbus_connection_dispatch (DBusConnection *connection)
  * should be that dbus_connection_set_watch_functions() has no effect,
  * but the add_function and remove_function may have been called.
  *
- * @todo We need to drop the lock when we call the
- * add/remove/toggled functions which can be a side effect
- * of setting the watch functions.
- * 
+ * @note For now, the thread lock on DBusConnection is held while
+ * watch functions are invoked, so inside these functions you
+ * may not invoke any methods on DBusConnection or it will deadlock.
+ * See the comments in the code or http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
+ * if you encounter this issue and want to attempt writing a patch.
+ *
  * @param connection the connection.
  * @param add_function function to begin monitoring a new descriptor.
  * @param remove_function function to stop monitoring a descriptor.
@@ -4571,42 +4609,17 @@ dbus_connection_set_watch_functions (DBusConnection              *connection,
                                      DBusFreeFunction             free_data_function)
 {
   dbus_bool_t retval;
-  DBusWatchList *watches;
 
   _dbus_return_val_if_fail (connection != NULL, FALSE);
   
   CONNECTION_LOCK (connection);
 
-#ifndef DBUS_DISABLE_CHECKS
-  if (connection->watches == NULL)
-    {
-      _dbus_warn_check_failed ("Re-entrant call to %s is not allowed\n",
-                               _DBUS_FUNCTION_NAME);
-      return FALSE;
-    }
-#endif
-  
-  /* ref connection for slightly better reentrancy */
-  _dbus_connection_ref_unlocked (connection);
-
-  /* This can call back into user code, and we need to drop the
-   * connection lock when it does. This is kind of a lame
-   * way to do it.
-   */
-  watches = connection->watches;
-  connection->watches = NULL;
-  CONNECTION_UNLOCK (connection);
-
-  retval = _dbus_watch_list_set_functions (watches,
+  retval = _dbus_watch_list_set_functions (connection->watches,
                                            add_function, remove_function,
                                            toggled_function,
                                            data, free_data_function);
-  CONNECTION_LOCK (connection);
-  connection->watches = watches;
   
   CONNECTION_UNLOCK (connection);
-  /* drop our paranoid refcount */
-  dbus_connection_unref (connection);
   
   return retval;
 }
@@ -4636,6 +4649,12 @@ dbus_connection_set_watch_functions (DBusConnection              *connection,
  * given remove_function.  The timer interval may change whenever the
  * timeout is added, removed, or toggled.
  *
+ * @note For now, the thread lock on DBusConnection is held while
+ * timeout functions are invoked, so inside these functions you
+ * may not invoke any methods on DBusConnection or it will deadlock.
+ * See the comments in the code or http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
+ * if you encounter this issue and want to attempt writing a patch.
+ * 
  * @param connection the connection.
  * @param add_function function to add a timeout.
  * @param remove_function function to remove a timeout.
@@ -4653,38 +4672,17 @@ dbus_connection_set_timeout_functions   (DBusConnection            *connection,
 					 DBusFreeFunction           free_data_function)
 {
   dbus_bool_t retval;
-  DBusTimeoutList *timeouts;
 
   _dbus_return_val_if_fail (connection != NULL, FALSE);
   
   CONNECTION_LOCK (connection);
-
-#ifndef DBUS_DISABLE_CHECKS
-  if (connection->timeouts == NULL)
-    {
-      _dbus_warn_check_failed ("Re-entrant call to %s is not allowed\n",
-                               _DBUS_FUNCTION_NAME);
-      return FALSE;
-    }
-#endif
   
-  /* ref connection for slightly better reentrancy */
-  _dbus_connection_ref_unlocked (connection);
-
-  timeouts = connection->timeouts;
-  connection->timeouts = NULL;
-  CONNECTION_UNLOCK (connection);
-  
-  retval = _dbus_timeout_list_set_functions (timeouts,
+  retval = _dbus_timeout_list_set_functions (connection->timeouts,
                                              add_function, remove_function,
                                              toggled_function,
                                              data, free_data_function);
-  CONNECTION_LOCK (connection);
-  connection->timeouts = timeouts;
   
   CONNECTION_UNLOCK (connection);
-  /* drop our paranoid refcount */
-  dbus_connection_unref (connection);
 
   return retval;
 }
@@ -5612,6 +5610,15 @@ dbus_connection_free_data_slot (dbus_int32_t *slot_p)
  * the connection is finalized. The slot number
  * must have been allocated with dbus_connection_allocate_data_slot().
  *
+ * @note As a temporary hack, this function does not take the
+ * main thread lock on DBusConnection, which allows it to be
+ * used from inside watch and timeout functions. (See the
+ * note in docs for dbus_connection_set_watch_functions().)
+ * A side effect of this is that you need to know there's
+ * a reference held on the connection while invoking
+ * dbus_connection_set_data(), or the connection could be
+ * finalized during dbus_connection_set_data().
+ * 
  * @param connection the connection
  * @param slot the slot number
  * @param data the data to store
@@ -5631,14 +5638,14 @@ dbus_connection_set_data (DBusConnection   *connection,
   _dbus_return_val_if_fail (connection != NULL, FALSE);
   _dbus_return_val_if_fail (slot >= 0, FALSE);
   
-  CONNECTION_LOCK (connection);
+  SLOTS_LOCK (connection);
 
   retval = _dbus_data_slot_list_set (&slot_allocator,
                                      &connection->slot_list,
                                      slot, data, free_data_func,
                                      &old_free_func, &old_data);
   
-  CONNECTION_UNLOCK (connection);
+  SLOTS_UNLOCK (connection);
 
   if (retval)
     {
@@ -5654,6 +5661,15 @@ dbus_connection_set_data (DBusConnection   *connection,
  * Retrieves data previously set with dbus_connection_set_data().
  * The slot must still be allocated (must not have been freed).
  *
+ * @note As a temporary hack, this function does not take the
+ * main thread lock on DBusConnection, which allows it to be
+ * used from inside watch and timeout functions. (See the
+ * note in docs for dbus_connection_set_watch_functions().)
+ * A side effect of this is that you need to know there's
+ * a reference held on the connection while invoking
+ * dbus_connection_get_data(), or the connection could be
+ * finalized during dbus_connection_get_data().
+ * 
  * @param connection the connection
  * @param slot the slot to get data from
  * @returns the data, or #NULL if not found
@@ -5666,13 +5682,13 @@ dbus_connection_get_data (DBusConnection   *connection,
 
   _dbus_return_val_if_fail (connection != NULL, NULL);
   
-  CONNECTION_LOCK (connection);
+  SLOTS_LOCK (connection);
 
   res = _dbus_data_slot_list_get (&slot_allocator,
                                   &connection->slot_list,
                                   slot);
   
-  CONNECTION_UNLOCK (connection);
+  SLOTS_UNLOCK (connection);
 
   return res;
 }