diff options
| -rw-r--r-- | dbus/dbus-connection.c | 206 |
1 files changed, 110 insertions, 96 deletions
diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 7f828c5f..6f38d141 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -75,6 +75,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" : \ @@ -263,6 +271,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. */ @@ -655,39 +664,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) - { - connection->watches = NULL; - _dbus_connection_ref_unlocked (connection); - CONNECTION_UNLOCK (connection); + if (connection->watches) + { 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; } else @@ -776,39 +788,42 @@ 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) - { - connection->timeouts = NULL; - _dbus_connection_ref_unlocked (connection); - CONNECTION_UNLOCK (connection); + if (connection->timeouts) + { 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 @@ -1269,6 +1284,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"); @@ -1345,6 +1364,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) @@ -2618,9 +2638,14 @@ 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 +#if 1 _dbus_atomic_inc (&connection->refcount); #else CONNECTION_LOCK (connection); @@ -2732,6 +2757,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); @@ -2766,9 +2793,14 @@ 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 +#if 1 last_unref = (_dbus_atomic_dec (&connection->refcount) == 1); #else CONNECTION_LOCK (connection); @@ -4819,9 +4851,11 @@ 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 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/tread.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. @@ -4840,42 +4874,18 @@ 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 is not allowed\n"); - 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; } @@ -4904,6 +4914,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 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. @@ -4921,37 +4937,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 is not allowed\n"); - 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; } @@ -5911,6 +5907,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 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 @@ -5930,14 +5935,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) { @@ -5953,6 +5958,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 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 @@ -5965,13 +5979,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; } |