[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-dbus
Subject:    Re: Out-of-memory error when using multiple threads
From:       "Olivier Hochreutiner" <olivier.hochreutiner () gmail ! com>
Date:       2007-12-31 13:19:16
Message-ID: f967eaa30712310519n38eeded2y85ede1313ecf113d () mail ! gmail ! com
[Download RAW message or body]

> Just keeping the lock is probably an improvement over what we have now,
> so I'd take that patch.

Here is a patch doing that. I also modified
dbus_connection_set_timeout_functions() as it seemed to be broken too.
I tried to 'make check' and there is something broken,   but also
without the patch (the error is different however). I am a bit
confused about the results, maybe you can try and see ?

> The ideal fix is probably rearranging the code a lot more to call out to
> the app in the right place (just before we'd be returning control to the
> app, like the other _and_unlock() functions), but it's a pretty big
> headache to do.

I don't have time do this right now but I'll write it somewhere on my
todo list...

Best,

Olivier

["timeout_race.patch" (text/x-patch)]

diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c
index 0de5f22..b2ef1bb 100644
--- a/dbus/dbus-connection.c
+++ b/dbus/dbus-connection.c
@@ -777,13 +777,13 @@ protected_change_timeout (DBusConnection           *connection,
    * drop lock and call out" one; but it has to be propagated up through all callers
    */
   
-  timeouts = connection->timeouts;
-  if (timeouts)
+  if (connection->timeouts)
     {
-      connection->timeouts = NULL;
-      _dbus_connection_ref_unlocked (connection);
-      CONNECTION_UNLOCK (connection);
-
+      /* Here we don't drop the connection lock to avoid that several threads try to use
+       * the timeouts field at the same time. This however forbids any call to dbus
+       * functions dealing with the connection in {add,remove,toogle}_function otherwise
+       * it would deadlock.
+       */
       if (add_function)
         retval = (* add_function) (timeouts, timeout);
       else if (remove_function)
@@ -797,10 +797,6 @@ protected_change_timeout (DBusConnection           *connection,
           (* toggle_function) (timeouts, timeout, enabled);
         }
       
-      CONNECTION_LOCK (connection);
-      connection->timeouts = timeouts;
-      _dbus_connection_unref_unlocked (connection);
-
       return retval;
     }
   else
@@ -4668,23 +4664,12 @@ dbus_connection_set_timeout_functions   (DBusConnection            *connection,
     }
 #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;
 }


_______________________________________________
dbus mailing list
dbus@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dbus


[prev in list] [next in list] [prev in thread] [next in thread]