From b0157af9a6d29eb0221d5cc746f25cd5da7a4497 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 18 Apr 2021 08:54:33 +0200 Subject: [PATCH 1/2] gdbus: document completion after idle action for g_dbus_connection_signal_unsubscribe() Since commit ab285899a6fb ('gdbusconnection: Document main context iteration for unsubscript'), we document when the user is guaranteed that all resources are gone after g_dbus_connection_signal_unsubscribe(). This is not merely an implementation detail, it's something that the user needs to be able to rely on. It is good that this is documented. However, libnm does something different ([1]). It registers to several D-Bus signals without providing a GDestroyNotify. After unsubscription, it schedules another idle action with lower priority and uses that to know when cleanup is complete. I think this is a useful alternative and should also be guaranteed and documented to work. Also note that this isn't just some implementation detail that currently happens to work. GDBusConnection tightly integrates with GMainContext and it works by scheduling idle sources with G_PRIORITY_DEFAULT priority. It needs to schedule all events with this same priority, otherwise the ordering is not preserved. At this point, with GDBusConnection working this way, this is no longer something that can reasonably be any different. It's how GDBusConnection fundamentally works, and a user must be able to rely on that. As such, this new promise isn't something that we would want to break in the future. Thus document it. [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/a55c10c6cb0cd047ff2aa835535d2abe09507ad6/src/libnm-client-impl/nm-client.c#L7918 --- gio/gdbusconnection.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index 28c6795bc..7c20462eb 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -3715,6 +3715,9 @@ unsubscribe_id_internal (GDBusConnection *connection, * g_dbus_connection_signal_subscribe() is called, in order to avoid memory * leaks through callbacks queued on the #GMainContext after it’s stopped being * iterated. + * Alternatively, any idle source with a priority lower than %G_PRIORITY_DEFAULT + * that was scheduled after unsubscription, also indicates that all resources + * of this subscription are released. * * Since: 2.26 */ From bdd6b753caf59e6aa066dc6645b5e584eb780784 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 18 Apr 2021 21:01:19 +0200 Subject: [PATCH 2/2] main: document notify function gets called during g_source_destroy() This seems non-obvious to me. Document it. It also seems important to know, because it means that the data pointer might already be destroyed, before the source is unreferenced for good. For example, if the data pointer keeps a reference to the GSource, and it might seem sensible to call: g_source_destroy(data->source); g_source_unref(data->source); /* <<< data is already destroyed? */ This leads to a crash, if the source was attached to a context. --- glib/gmain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/glib/gmain.c b/glib/gmain.c index 2e1ab3a25..4fe587a67 100644 --- a/glib/gmain.c +++ b/glib/gmain.c @@ -1370,6 +1370,10 @@ g_source_destroy_internal (GSource *source, * * This function is safe to call from any thread, regardless of which thread * the #GMainContext is running in. + * + * If the source is currently attached to a #GMainContext, destroying it + * will effectively unset the callback similar to calling g_source_set_callback(). + * This can mean, that the data's #GDestroyNotify gets called right away. */ void g_source_destroy (GSource *source) @@ -1761,6 +1765,9 @@ g_source_set_callback_indirect (GSource *source, * It is safe to call this function multiple times on a source which has already * been attached to a context. The changes will take effect for the next time * the source is dispatched after this call returns. + * + * Note that g_source_destroy() for a currently attached source has the effect + * of also unsetting the callback. **/ void g_source_set_callback (GSource *source,