Index: ChangeLog =================================================================== RCS file: /cvs/dbus/dbus/ChangeLog,v retrieving revision 1.895 diff -u -p -r1.895 ChangeLog --- ChangeLog 8 Sep 2005 18:54:42 -0000 1.895 +++ ChangeLog 10 Sep 2005 20:27:24 -0000 @@ -0,0 +1,46 @@ +2005-09-10 Mark McLoughlin + + Remove requirement that you must call dbus_connection_close() + before dropping the last ref on DBusConnections. + + * dbus/dbus-connection.c: + (dbus_connection_unref_unlocked): add comment about why + this function shouldn't exist and add an assertion + which will be triggered if it causes finalization. + (connection_lookup_shared): update comment about why its + safe to ref here. + (connection_record_shared): rename so that unlocked() refers + to the shared_connections lock. + (connection_forget_shared_unlocked): don't take the + shared_connections lock here. + (connection_forget_shared): but do take it here. + (_dbus_connection_open_internal): don't need to close + the connection before unreffing it anymore. + (_dbus_connection_last_unref): remove assertion about + us needing to be disconnected at this point. + (dbus_connection_unref): take the shared_connections + lock and remove it from the shared_connections table + if we're finalizing; add horrible hack to allow us + to close the connection without the connection lock. + + * dbus/dbus-bus.c: (dbus_bus_get): no need to close + the connection before unrefing it. + + * dbus/dbus-server-unix.c: (handle_new_client_fd_and_unlock): + don't leak a server ref and remove TODOs. + + * bus/bus.c: (new_connection_callback): no need to close + the connection before unrefing it. + + * bus/connection.c: (bus_connections_unref): no need to + do all this refing, unrefing and closing of the connections. + + * bus/dispatch.c: (check_hello_connection): no need to + close the connection before unrefing it. + + * glib/examples/example-client.c: (main): + glib/examples/example-service.c: (main): + glib/examples/example-signal-emitter.c: (main): + glib/examples/example-signal-recipient.c: (main): unref + things before quiting. + Index: dbus/dbus-connection.c =================================================================== RCS file: /cvs/dbus/dbus/dbus/dbus-connection.c,v retrieving revision 1.110 diff -u -p -r1.110 dbus/dbus-connection.c --- dbus/dbus-connection.c 26 Aug 2005 17:34:59 -0000 1.110 +++ dbus/dbus-connection.c 10 Sep 2005 20:20:50 -0000 @@ -123,13 +123,13 @@ * entire outgoing queue. The GLib/Qt add-on libraries again * handle the details here for you by setting up watch functions. * - * When a connection is disconnected, you are guaranteed to get a - * signal "Disconnected" from the interface - * #DBUS_INTERFACE_LOCAL, path - * #DBUS_PATH_LOCAL. - * - * You may not drop the last reference to a #DBusConnection - * until that connection has been disconnected. + * When a connection is either explicitly disconnected or disconnected + * by the other side, you are guaranteed to get a signal "Disconnected" + * from the interface #DBUS_INTERFACE_LOCAL, path #DBUS_PATH_LOCAL. + * + * However, if you drop the last reference to a #DBusConnection before + * the connection has been disconnected, you will not get a "Disconected" + * signal. * * You may dispatch the unprocessed incoming message queue even if the * connection is disconnected. However, "Disconnected" will always be @@ -1267,6 +1269,12 @@ _dbus_connection_ref_unlocked (DBusConne * Decrements the reference count of a DBusConnection. * Requires that the caller already holds the connection lock. * + * @todo this function basically makes no sense; if we finalize + * with the connection lock held, then the caller is going + * going to crash when trying to unlock it and it doesn't look + * like any of the callers can be sure this isn't the last + * ref + * * @param connection the connection. */ void @@ -1275,27 +1283,15 @@ _dbus_connection_unref_unlocked (DBusCon dbus_bool_t last_unref; HAVE_LOCK_CHECK (connection); - - _dbus_assert (connection != NULL); - /* The connection lock is better than the global - * lock in the atomic increment fallback - */ - -#ifdef DBUS_HAVE_ATOMIC_INT - last_unref = (_dbus_atomic_dec (&connection->refcount) == 1); -#else + _dbus_assert (connection != NULL); _dbus_assert (connection->refcount.value > 0); connection->refcount.value -= 1; - last_unref = (connection->refcount.value == 0); -#if 0 - printf ("unref_unlocked() connection %p count = %d\n", connection, connection->refcount.value); -#endif -#endif - + last_unref = (connection->refcount.value == 0); + if (last_unref) - _dbus_connection_last_unref (connection); + _dbus_assert_not_reached ("Tell me ... just how are you going to unlock me now that I'm dead and gone?"); } static dbus_uint32_t @@ -1423,13 +1419,9 @@ connection_lookup_shared (DBusAddressEnt if (*result) { - /* The DBusConnection can't have been disconnected - * between the lookup and this code, because the - * disconnection will take the shared_connections lock to - * remove the connection. It can't have been finalized - * since you have to disconnect prior to finalize. - * - * Thus it's safe to ref the connection. + /* We hold the shared_connections lock in unref() + * so it can't have been finalized between the lookup + * and this code. */ dbus_connection_ref (*result); @@ -1444,8 +1436,8 @@ connection_lookup_shared (DBusAddressEnt } static dbus_bool_t -connection_record_shared_unlocked (DBusConnection *connection, - const char *guid) +connection_record_shared (DBusConnection *connection, + const char *guid) { char *guid_key; char *guid_in_connection; @@ -1496,22 +1488,31 @@ connection_record_shared_unlocked (DBusC static void connection_forget_shared_unlocked (DBusConnection *connection) { - HAVE_LOCK_CHECK (connection); - + /* We may get run here without the connection lock held, + * but only in the case of finalize where we should have + * no thread related issues. + */ + if (connection->server_guid == NULL) return; _dbus_verbose ("dropping connection to %s out of the shared table\n", connection->server_guid); - _DBUS_LOCK (shared_connections); - if (!_dbus_hash_table_remove_string (shared_connections, connection->server_guid)) _dbus_assert_not_reached ("connection was not in the shared table"); dbus_free (connection->server_guid); connection->server_guid = NULL; +} + +static void +connection_forget_shared (DBusConnection *connection) +{ + _DBUS_LOCK (shared_connections); + + connection_forget_shared_unlocked (connection); _DBUS_UNLOCK (shared_connections); } @@ -1610,10 +1611,9 @@ _dbus_connection_open_internal (const ch */ if (guid && - !connection_record_shared_unlocked (connection, guid)) + !connection_record_shared (connection, guid)) { _DBUS_SET_OOM (&tmp_error); - dbus_connection_close (connection); dbus_connection_unref (connection); connection = NULL; } @@ -1781,13 +1781,11 @@ _dbus_connection_last_unref (DBusConnect _dbus_verbose ("Finalizing connection %p\n", connection); _dbus_assert (connection->refcount.value == 0); - - /* You have to disconnect the connection before unref:ing it. Otherwise - * you won't get the disconnected message. - */ - _dbus_assert (!_dbus_transport_get_is_connected (connection->transport)); _dbus_assert (connection->server_guid == NULL); - +#ifndef DBUS_DISABLE_CHECKS + _dbus_assert (!connection->have_connection_lock); +#endif + /* ---- We're going to call various application callbacks here, hope it doesn't break anything... */ _dbus_object_tree_free_all_unlocked (connection->objects); @@ -1862,12 +1860,7 @@ _dbus_connection_last_unref (DBusConnect /** * Decrements the reference count of a DBusConnection, and finalizes - * it if the count reaches zero. It is a bug to drop the last reference - * to a connection that has not been disconnected. - * - * @todo in practice it can be quite tricky to never unref a connection - * that's still connected; maybe there's some way we could avoid - * the requirement. + * it if the count reaches zero. * * @param connection the connection. */ @@ -1878,7 +1871,12 @@ dbus_connection_unref (DBusConnection *c _dbus_return_if_fail (connection != NULL); _dbus_return_if_fail (connection->generation == _dbus_current_generation); - + + /* Make sure no-one looks us up from shared_connections + * until we're sure we're going not to finalize. + */ + _DBUS_LOCK (shared_connections); + /* The connection lock is better than the global * lock in the atomic increment fallback */ @@ -1901,7 +1899,30 @@ dbus_connection_unref (DBusConnection *c #endif if (last_unref) - _dbus_connection_last_unref (connection); + connection_forget_shared_unlocked (connection); + + _DBUS_UNLOCK (shared_connections); + + if (last_unref) + { + /* FIXME: this bit sucks; if we fixed _dbus_connection_remove_watch() + * to not require the lock to be held we and not ref/unref the + * connection, then we could just remove this crackrock and let + * _dbus_transport_unref() in last_unref() close the connection + */ + if (_dbus_transport_get_is_connected (connection->transport)) + { + CONNECTION_LOCK (connection); + connection->refcount.value = 1; + + _dbus_transport_disconnect (connection->transport); + + connection->refcount.value = 0; + CONNECTION_UNLOCK (connection); + } + + _dbus_connection_last_unref (connection); + } } /** @@ -3256,7 +3277,7 @@ _dbus_connection_get_dispatch_status_unl _dbus_verbose ("Sending disconnect message from %s\n", _DBUS_FUNCTION_NAME); - connection_forget_shared_unlocked (connection); + connection_forget_shared (connection); /* We haven't sent the disconnect message already, * and all real messages have been queued up. Index: dbus/dbus-bus.c =================================================================== RCS file: /cvs/dbus/dbus/dbus/dbus-bus.c,v retrieving revision 1.44 diff -u -p -r1.44 dbus-bus.c --- dbus/dbus-bus.c 2 Aug 2005 11:26:59 -0000 1.44 +++ dbus/dbus-bus.c 10 Sep 2005 20:02:29 -0000 @@ -314,7 +314,8 @@ ensure_bus_data (DBusConnection *connect * connection to the bus already exists, then that connection is * returned. Caller owns a reference to the bus. * - * @todo alex thinks we should nullify the connection when we get a disconnect-message. + * @todo alex thinks we should nullify the connection when we get a + * disconnect-message as well as when the connection is finalized * * @param type bus type * @param error address where an error can be returned. @@ -392,7 +393,6 @@ dbus_bus_get (DBusBusType type, if (!dbus_bus_register (connection, error)) { _DBUS_ASSERT_ERROR_IS_SET (error); - dbus_connection_close (connection); dbus_connection_unref (connection); _DBUS_UNLOCK (bus); Index: dbus/dbus-server-unix.c =================================================================== RCS file: /cvs/dbus/dbus/dbus/dbus-server-unix.c,v retrieving revision 1.28 diff -u -p -r1.28 dbus-server-unix.c --- dbus/dbus-server-unix.c 26 Feb 2005 06:37:46 -0000 1.28 +++ dbus/dbus-server-unix.c 10 Sep 2005 20:02:32 -0000 @@ -71,14 +71,6 @@ unix_finalize (DBusServer *server) dbus_free (server); } -/** - * @todo unreffing the connection at the end may cause - * us to drop the last ref to the connection before - * disconnecting it. That is invalid. - * - * @todo doesn't this leak a server refcount if - * new_connection_function is NULL? - */ /* Return value is just for memory, not other failures. */ static dbus_bool_t handle_new_client_fd_and_unlock (DBusServer *server, @@ -142,8 +134,9 @@ handle_new_client_fd_and_unlock (DBusSer { (* new_connection_function) (server, connection, new_connection_data); - dbus_server_unref (server); } + + dbus_server_unref (server); /* If no one grabbed a reference, the connection will die. */ dbus_connection_unref (connection); Index: bus/bus.c =================================================================== RCS file: /cvs/dbus/dbus/bus/bus.c,v retrieving revision 1.64 diff -u -p -r1.64 bus.c --- bus/bus.c 16 Jun 2005 06:05:09 -0000 1.64 +++ bus/bus.c 10 Sep 2005 20:07:04 -0000 @@ -169,25 +169,20 @@ new_connection_callback (DBusServer { BusContext *context = data; - if (!bus_connections_setup_connection (context->connections, new_connection)) + if (bus_connections_setup_connection (context->connections, new_connection)) { - _dbus_verbose ("No memory to setup new connection\n"); + dbus_connection_set_max_received_size (new_connection, + context->limits.max_incoming_bytes); - /* if we don't do this, it will get unref'd without - * being disconnected... kind of strange really - * that we have to do this, people won't get it right - * in general. - */ - dbus_connection_close (new_connection); + dbus_connection_set_max_message_size (new_connection, + context->limits.max_message_size); } - - dbus_connection_set_max_received_size (new_connection, - context->limits.max_incoming_bytes); - - dbus_connection_set_max_message_size (new_connection, - context->limits.max_message_size); + else + { + _dbus_verbose ("No memory to setup new connection\n"); - /* on OOM, we won't have ref'd the connection so it will die. */ + /* we won't have ref'd the connection so it will die. */ + } } static void Index: bus/connection.c =================================================================== RCS file: /cvs/dbus/dbus/bus/connection.c,v retrieving revision 1.61 diff -u -p -r1.61 connection.c --- bus/connection.c 3 Aug 2005 17:42:56 -0000 1.61 +++ bus/connection.c 10 Sep 2005 20:02:25 -0000 @@ -491,31 +491,13 @@ bus_connections_unref (BusConnections *c { /* drop all incomplete */ while (connections->incomplete != NULL) - { - DBusConnection *connection; - - connection = connections->incomplete->data; - - dbus_connection_ref (connection); - dbus_connection_close (connection); - bus_connection_disconnected (connection); - dbus_connection_unref (connection); - } + bus_connection_disconnected (connections->incomplete->data); _dbus_assert (connections->n_incomplete == 0); /* drop all real connections */ while (connections->completed != NULL) - { - DBusConnection *connection; - - connection = connections->completed->data; - - dbus_connection_ref (connection); - dbus_connection_close (connection); - bus_connection_disconnected (connection); - dbus_connection_unref (connection); - } + bus_connection_disconnected (connections->completed->data); _dbus_assert (connections->n_completed == 0); Index: bus/dispatch.c =================================================================== RCS file: /cvs/dbus/dbus/bus/dispatch.c,v retrieving revision 1.71 diff -u -p -r1.71 dispatch.c --- bus/dispatch.c 15 Jul 2005 15:21:43 -0000 1.71 +++ bus/dispatch.c 10 Sep 2005 20:02:27 -0000 @@ -1490,7 +1489,6 @@ check_hello_connection (BusContext *cont if (!bus_setup_debug_client (connection)) { - dbus_connection_close (connection); dbus_connection_unref (connection); return TRUE; } Index: glib/examples/example-client.c =================================================================== RCS file: /cvs/dbus/dbus/glib/examples/example-client.c,v retrieving revision 1.2 diff -u -p -r1.2 example-client.c --- glib/examples/example-client.c 11 Jul 2005 16:12:49 -0000 1.2 +++ glib/examples/example-client.c 10 Sep 2005 20:02:32 -0000 @@ -68,18 +68,30 @@ main (int argc, char **argv) if (!dbus_g_proxy_call (remote_object, "HelloWorld", &error, G_TYPE_STRING, "Hello from example-client.c!", G_TYPE_INVALID, G_TYPE_STRV, &reply_list, G_TYPE_INVALID)) - lose_gerror ("Failed to complete HelloWorld", error); + { + g_object_unref (remote_object); + dbus_g_connection_unref (bus); + lose_gerror ("Failed to complete HelloWorld", error); + } if (!dbus_g_proxy_call (remote_object, "GetTuple", &error, G_TYPE_INVALID, G_TYPE_VALUE_ARRAY, &hello_reply_struct, G_TYPE_INVALID)) - lose_gerror ("Failed to complete GetTuple", error); + { + g_object_unref (remote_object); + dbus_g_connection_unref (bus); + lose_gerror ("Failed to complete GetTuple", error); + } if (!dbus_g_proxy_call (remote_object, "GetDict", &error, G_TYPE_INVALID, DBUS_TYPE_G_STRING_STRING_HASHTABLE, &hello_reply_dict, G_TYPE_INVALID)) - lose_gerror ("Failed to complete GetDict", error); + { + g_object_unref (remote_object); + dbus_g_connection_unref (bus); + lose_gerror ("Failed to complete GetDict", error); + } printf ("reply_list: "); for (reply_ptr = reply_list; *reply_ptr; reply_ptr++) @@ -110,12 +122,19 @@ main (int argc, char **argv) if (!dbus_g_proxy_call (remote_object_introspectable, "Introspect", &error, G_TYPE_INVALID, G_TYPE_STRING, &introspect_data, G_TYPE_INVALID)) - lose_gerror ("Failed to complete Introspect", error); + { + g_object_unref (remote_object_introspectable); + g_object_unref (remote_object); + dbus_g_connection_unref (bus); + lose_gerror ("Failed to complete Introspect", error); + } + printf ("%s", introspect_data); g_free (introspect_data); - g_object_unref (G_OBJECT (remote_object_introspectable)); - g_object_unref (G_OBJECT (remote_object)); + g_object_unref (remote_object_introspectable); + g_object_unref (remote_object); + dbus_g_connection_unref (bus); exit(0); } Index: glib/examples/example-service.c =================================================================== RCS file: /cvs/dbus/dbus/glib/examples/example-service.c,v retrieving revision 1.3 diff -u -p -r1.3 example-service.c --- glib/examples/example-service.c 11 Jul 2005 16:12:49 -0000 1.3 +++ glib/examples/example-service.c 10 Sep 2005 20:02:32 -0000 @@ -139,7 +139,11 @@ main (int argc, char **argv) G_TYPE_INVALID, G_TYPE_UINT, &request_name_result, G_TYPE_INVALID)) - lose_gerror ("Failed to acquire org.designfu.SampleService", error); + { + g_object_unref (bus_proxy); + dbus_g_connection_unref (bus); + lose_gerror ("Failed to acquire org.designfu.SampleService", error); + } obj = g_object_new (SOME_TYPE_OBJECT, NULL); @@ -149,5 +153,9 @@ main (int argc, char **argv) g_main_loop_run (mainloop); + g_object_unref (obj); + g_object_unref (bus_proxy); + dbus_g_connection_unref (bus); + exit (0); } Index: glib/examples/example-signal-emitter.c =================================================================== RCS file: /cvs/dbus/dbus/glib/examples/example-signal-emitter.c,v retrieving revision 1.2 diff -u -p -r1.2 example-signal-emitter.c --- glib/examples/example-signal-emitter.c 8 Jul 2005 16:25:34 -0000 1.2 +++ glib/examples/example-signal-emitter.c 10 Sep 2005 20:02:32 -0000 @@ -118,7 +118,11 @@ main (int argc, char **argv) G_TYPE_INVALID, G_TYPE_UINT, &request_name_result, G_TYPE_INVALID)) - lose_gerror ("Failed to acquire org.designfu.TestService", error); + { + g_object_unref (bus_proxy); + dbus_g_connection_unref (bus); + lose_gerror ("Failed to acquire org.designfu.TestService", error); + } obj = g_object_new (TEST_TYPE_OBJECT, NULL); @@ -128,5 +132,9 @@ main (int argc, char **argv) g_main_loop_run (mainloop); + g_object_unref (obj); + g_object_unref (bus_proxy); + dbus_g_connection_unref (bus); + exit (0); } Index: glib/examples/example-signal-recipient.c =================================================================== RCS file: /cvs/dbus/dbus/glib/examples/example-signal-recipient.c,v retrieving revision 1.2 diff -u -p -r1.2 example-signal-recipient.c --- glib/examples/example-signal-recipient.c 29 Jun 2005 16:58:59 -0000 1.2 +++ glib/examples/example-signal-recipient.c 10 Sep 2005 20:02:32 -0000 @@ -65,7 +65,10 @@ main (int argc, char **argv) "/org/designfu/TestService/object", "org.designfu.TestService"); if (!remote_object) - lose_gerror ("Failed to get name owner", error); + { + dbus_g_connection_unref (bus); + lose_gerror ("Failed to get name owner", error); + } /* IMPORTANT: * @@ -98,5 +101,8 @@ main (int argc, char **argv) g_main_loop_run (mainloop); + g_object_unref (remote_object); + dbus_g_connection_unref (bus); + exit (0); }