GNOME Bugzilla – Bug 742618
Add transfer floating alias for input parameters
Last modified: 2017-05-17 21:07:22 UTC
Created attachment 294124 [details] [review] Add a test for "(in) (transfer floating)" parameters beeing an alias for "(in) (transfer none)". The "(in)" alias was (unintenionally?) introduced in https://git.gnome.org/browse/gobject-introspection/commit/?id=699ad0fec427c79bec1 which added an alias for return annotations. This change makes sure that this continues to work. Functions which ref_sink in params are currently marked as transfer-none since in the case of non-floating objects (which all bindings use) no ownership gets transfered. But in case of floating objects, which is the common case when using the C API directly, the ownership _is_ transfered. Using transfer-floating should make this clearer while giving the same result for bindings. Functions where this could be used: gst_bin_add, gtk_container_add, gst_element_add_pad See https://bugzilla.gnome.org/show_bug.cgi?id=657202 and https://bugzilla.gnome.org/show_bug.cgi?id=702960
My 2¢ without any plan to make any actual contribution to this bug: I'd prefer if this were called (transfer consume). I'd also prefer that we think, at the same time, about how to label return values that deal in floating references. The common case is where a floating reference is returned and needs to be sunk if you want to hold on to it. That's probably what I would imagine (transfer floating) means. Another less common case is where either a floating reference or a full reference is returned. This is usually the case on the return value of callbacks that may want to return either a floating or a strong reference. I don't have a great name for that, but I might call it (transfer take). For GVariant you deal with that by calling g_variant_take_ref(). For GObject it is dealt with by checking is_floating() and calling sink(), but it may be worth adding a g_object_take_ref() helper as well.
(In reply to comment #1) > My 2¢ without any plan to make any actual contribution to this bug: Thanks, appreciated. > I'd prefer if this were called (transfer consume). > > I'd also prefer that we think, at the same time, about how to label return > values that deal in floating references. > > The common case is where a floating reference is returned and needs to be sunk > if you want to hold on to it. That's probably what I would imagine (transfer > floating) means. Let me elaborate: Some definitions: Source/Target: Source is the source of the transfer, target the target of the transfer (for both callbacks, functions, params and returns) Ownership: The responsibility to destroy a reference by either unrefing it, passing it on with transfer-full or attach it to some owned field (where finalizing the parent value will finalize its owned fields) Floating-Ownership: The responsibility to remove the floating flag of an object, either by doing it yourself or passing it to some other code which guarantees to remove the flag for you. You create a floating-ownership by force_float() an object. After that you have the responsibility to get rid of it again either by calling ref_sink() or passing it to a functions which does so for you. After removing the floating flag, the reference of the code calling force_float() gets stolen, and the floating-ownership gets converted to a normal ownership i.e. the responsibility to unref(). The suggested "transfer-floating" annotation specifies that the target will ref_sink() any passed floating object, either by itself or by passing it to another function which also has transfer-floating. This is true for in params as well as return values and out args (which is why I think transfer-floating is the right name for both cases) In case one passes a non-floating object to transfer-floating it is equal to transfer-none (since ref_sink is ref in this case), thus the alias to transfer-none for bindings. > Another less common case is where either a floating reference or a full > reference is returned. This is usually the case on the return value of > callbacks that may want to return either a floating or a strong reference. I > don't have a great name for that, but I might call it (transfer take). For > GVariant you deal with that by calling g_variant_take_ref(). For GObject it is > dealt with by checking is_floating() and calling sink(), but it may be worth > adding a g_object_take_ref() helper as well. In this case the semantics are the same for floating object as described by transfer-floating (take_ref is the same as ref_sink). But in the case of non-floating objects we get transfer-full semantics. I suggest to add a transfer-floating-full annotation for this case which is an alias for transfer-full. To summarize: transfer-floating: The target guarantees that it will call ref_sink() or equivalent on the passed object or will find someone who does. In case of a non-floating object this is equivalent to transfer-none. transfer-floating-full: The target guarantees that it will call take_ref() or equivalent on the passed object or will find someone who does. In case of a non-floating object this is equivalent to transfer-full. Any code which receives an object through a transfer-floating or transfer-floating-full annotated param/return has to make sure that it a) calls ref_sink or take_ref itself or b) returns / passes it to a function with a transfer-floating or transfer-floating-full annotation. Typelib: If this would ever be integrated into the typelib this would also allow for a backwards compatible implementation by adding one additional flag (if there is space for that..). transfer-floating could be specified as transfer-none + floating flag. transfer-floating-full could be specified as transfer-full + floating flag. The transfers are the same as currently used by bindings if the floating flag is ignored. The description of the typelib floating flag would be: In case a floating objects gets transfered, it is guaranteed that the floating flag will be removed and the reference stolen. disclaimer: I have never used floating refs much, so I might be missing something obvious..
(In reply to comment #2) > The suggested "transfer-floating" annotation specifies that the target will > ref_sink() any passed floating object, either by itself or by passing it to > another function which also has transfer-floating. This is true for in params > as well as return values and out args (which is why I think transfer-floating > is the right name for both cases) This is an extremely consistent and logical way to think about this -- I only wish it were as intuitive from the standpoint of a user who is simply reading the tag in the docs. One more weird case, just for completeness: functions that sometimes return no refs at all instead of returning a floating ref. g_object_new() on widgets is an example of this (when used on GtkWindow for example) and so is g_variant_new_va(). In both case, the correct behaviour is to always call the 'ref sink' function. Here's the docs from g_variant_new_va(): """ The return value will be floating if it was a newly created GVariant instance (for example, if the format string was "(ii)"). In the case that the format_string was '*', '?', 'r', or a format starting with '@' then the collected GVariant pointer will be returned unmodified, without adding any additional references. In order to behave correctly in all cases it is necessary for the calling function to g_variant_ref_sink() the return result before returning control to the user that originally provided the pointer. At this point, the caller will have their own full reference to the result. This can also be done by adding the result to a container, or by passing it to another g_variant_new() call. ... Returns: a new, usually floating, GVariant """
there's a long discussion about the transfer: floating annotation in bug 657202 — it should be referenced here.
(In reply to comment #3) > (In reply to comment #2) > > The suggested "transfer-floating" annotation specifies that the target will > > ref_sink() any passed floating object, either by itself or by passing it to > > another function which also has transfer-floating. This is true for in params > > as well as return values and out args (which is why I think transfer-floating > > is the right name for both cases) > > This is an extremely consistent and logical way to think about this -- I only > wish it were as intuitive from the standpoint of a user who is simply reading > the tag in the docs. > > One more weird case, just for completeness: functions that sometimes return no > refs at all instead of returning a floating ref. g_object_new() on widgets is > an example of this (when used on GtkWindow for example) and so is > g_variant_new_va(). In both case, the correct behaviour is to always call the > 'ref sink' function. The first sentence should have read "the target will ref_sink() any passed object" (not just floating, see the summary). Does this cover those cases?
It does cover those cases. I only included my comment 'for completeness'.
Some floating "weirdness" in GStreamer: * GstTaskPool, GstTask, GstClock, GstBus, GstCollectPads, sink themselfs in _init despite being GInitiallyUnowned. So g_object_new is transfer-full in those cases without any way to know it. As a result they all leak in Python. * GstBufferPool is floating, it gets added to a GstQuery but never sunk. gst_query_parse_nth_allocation_pool() can return floating objects. * GstDeviceMonitor is never sunk. Not sure what to do here...
Because I haven't mentioned it above: The return transfer annotation for g_object_new() would be transfer-floating in case of GInitiallyUnowned and transfer-full otherwise.
I've opened bug 743062 for the leaks of GInitiallyUnowned subclasses in GStreamer.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Since there has been some activity in gstreamer referencing this (bug 782499): The patch here still applies on master and passes tests.
Comment on attachment 294124 [details] [review] Add a test for "(in) (transfer floating)" parameters beeing an alias for "(in) (transfer none)". GStreamer has started to use transfer-floating now: https://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=30f871d274e8a544779c287a1a4c188f22c2066f Since this is just a small test and doesn't change any behavior I went ahead and pushed it.