GNOME Bugzilla – Bug 767952
g_dbus_method_invocation_return_*, g_dbus_method_invocation_take_error: They do unref invocation, but that is not the whole story
Last modified: 2016-11-22 17:13:19 UTC
The documentation for the g_dbus_method_invocation_return_* family of methods and g_dbus_method_invocation_take_error has this mysterious line: "This method will free invocation, you cannot use it afterwards." While it is true that that those methods unref the GDBusMethodInvocation instance, it is not the entire story. When GDBusConnection calls the GDBusInterfaceMethodCallFunc member of GDBusInterfaceVTable (ie. vtable->method_call) it adds an extra reference to the invocation. Therefore, code that implements a D-Bus service and wants to handle a method call already has this extra reference on the invocation added to it. When this code calls g_dbus_method_invocation_return_* or g_dbus_method_invocation_take_error, it is this extra reference which is being dropped. If, for whatever reason, the code handling the method call adds its own references to the invocation, then those still need to be dropped separately. I wish the documentation was clearer in this regard. One option is to say that code handling the method call does not need to add its own references to the GDBusMethodInvocation to keep it alive across an asynchronous operation because GDBusConnection already holds an extra ref, which will be dropped by g_dbus_method_invocation_return_* and g_dbus_method_invocation_take_error. However, I don't know what is the best way or place to express this. Or we can just drop the line about the invocation being freed. One can argue that the ref/unref calls inside GDBus are an internal detail that users need not know about. They can add their own refs and drop them as they would for any other GObject.
so the docs do say this: Ownership of the GDBusMethodInvocation object passed to the method_call() function is transferred to your handler; you must call one of the methods of GDBusMethodInvocation to return a reply (possibly empty), or an error. These functions also take ownership of the passed-in invocation object…Calling one of these functions may be done within your method_call() implementation but it also can be done at a later point to handle the method asynchronously. Granted, because of all the layers and codegen that is involved, there's a bit of a gap between where the docs are and where the method calls are handled. (Before seeing your message come in, I certainly thought users wishing to reply asynchronously needed to take a reference)
Created attachment 330241 [details] [review] GDBusMethodInvocation: Clarify how the ownership is handled
(In reply to Ray Strode [halfline] from comment #1) > so the docs do say this: > > Ownership of the GDBusMethodInvocation object passed to the method_call() > function is transferred to your handler; you must call one of the methods of > GDBusMethodInvocation to return a reply (possibly empty), or an error. These > functions also take ownership of the passed-in invocation object…Calling one > of these functions may be done within your method_call() implementation but > it also can be done at a later point to handle the method asynchronously. Nice. I am guilty of never reading the GDBusInterfaceVTable carefully. How about pointing to GDBusMethodInvocation from the g_dbus_method_take*/_return* documentation, and changing "you cannot use it afterwards" to something about ownership. It does sound strange to say "This method will free @invocation" for a reference counted object. Someone can always hold a ref and it will just continue to stay alive. Perhaps, something like: "This method will take ownership of @invocation. See #GDBusInterfaceVTable for more information about the ownership of @invocation." > (Before seeing your message come in, I certainly thought users wishing to > reply asynchronously needed to take a reference) Yes, I do the same thing (and intend to continue doing so) to avoid needless divergences from usual GObject practice. Long ago I had read the GDBus code to teach myself what is actually going on, but it cropped up when reviewing code from a new contributor. I guess it is better to fix the documentation than having to explain this over and over again. :)
Review of attachment 330241 [details] [review]: ::: gio/gdbusmethodinvocation.c @@ +537,3 @@ * It is an error if @parameters is not of the right format. * + * This method will take ownership of @invocation. See So to me, this new text is better than the old text, but it also seems to just be restating the "(transfer full)" in prose, so on the surface it doesn't seem like it would add much? On the other hand, that "(transfer full)" is obviously not getting noticed, so maybe it is a net win. It's certainly no more redundant than the text that was there before anyway. Oddly, the transfer annotation doesn't seem to show up in devhelp, which might be part of the problem. It does show up at https://developer.gnome.org/gio/2.49/GDBusMethodInvocation.html#g-dbus-method-invocation-return-value though. @@ +539,3 @@ + * This method will take ownership of @invocation. See + * #GDBusInterfaceVTable for more information about the ownership of + * @invocation. i think this is a really good idea.
Ping. Here is an example of this bug in the wild: https://bugzilla.gnome.org/show_bug.cgi?id=756431#c6
rishi, i'd just push this, i don't think anyone is going to complain.
Ok, pushed to master.