GNOME Bugzilla – Bug 698640
Does not call dbus_connection_close on cleanup
Last modified: 2013-05-03 15:49:31 UTC
While running integration tests against current git head, I noticed this message on shutdown of NM: ------------- 8< -------------- NetworkManager[31415]: <info> caught signal 15, shutting down normally. process 31415: The last reference on a connection was dropped without closing the connection. This is a bug in an application. See dbus_connection_unref() documentation for details. Most likely, the application was supposed to call dbus_connection_close(), since this is a private connection. this watch should have been invalidatedthis watch should have been invalidatedNetworkManager[31415]: <info> exiting (success) ------------- 8< -------------- I think this is a regression from http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a87b5a15df45d7974c942ffa6bc4a5453c269d4e . In the case of a private connection, dbus_connection_unref() isn't enough, one needs to dbus_connection_close() the connection first. See http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga6385ff09bc108238c4429e7c195dab25
Created attachment 242197 [details] [review] WIP: Close private D-BUS connections This fixes the message, but I have a feeling that this might not be sufficient. a87b5a1's commit message indicates that sometimes NM actually uses shared connections, in which case we mustn't close them?
The private server stuff will never be a shared connection actually so I think the patch is correct. Need more info from people who know dbus better though...
Review of attachment 242197 [details] [review]: My memory of libdbus is fairly dim now, but the patch looks reasonable to me. It'd probably be cleaner if we could arrange for dbus_connection_close() to be explicitly called exactly once in the right place, but eh.
Looking at the dbus code for DBusServer instances quickly, on the *server* side I believe all the sockets are non-shared. Only in on the client side are they shared. So I think the patch is correct.
Pushed to master with a small fixup; we can remove the dbus_connection_close() in the Disconnected handle because the connection will be removed from the hash table, thus closing it too. Thanks!