After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 742618 - Add transfer floating alias for input parameters
Add transfer floating alias for input parameters
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
Depends on:
Blocks: 758903
 
 
Reported: 2015-01-08 21:28 UTC by Christoph Reiter (lazka)
Modified: 2017-05-17 21:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a test for "(in) (transfer floating)" parameters beeing an alias for "(in) (transfer none)". (6.91 KB, patch)
2015-01-08 21:28 UTC, Christoph Reiter (lazka)
committed Details | Review

Description Christoph Reiter (lazka) 2015-01-08 21:28:52 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
Comment 1 Allison Karlitskaya (desrt) 2015-01-08 22:16:06 UTC
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.
Comment 2 Christoph Reiter (lazka) 2015-01-09 12:16:34 UTC
(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..
Comment 3 Allison Karlitskaya (desrt) 2015-01-09 14:16:34 UTC
(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


"""
Comment 4 Emmanuele Bassi (:ebassi) 2015-01-09 14:41:29 UTC
there's a long discussion about the transfer: floating annotation in bug 657202 — it should be referenced here.
Comment 5 Christoph Reiter (lazka) 2015-01-10 01:06:11 UTC
(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?
Comment 6 Allison Karlitskaya (desrt) 2015-01-10 21:39:18 UTC
It does cover those cases.  I only included my comment 'for completeness'.
Comment 7 Christoph Reiter (lazka) 2015-01-14 17:47:42 UTC
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...
Comment 8 Christoph Reiter (lazka) 2015-01-14 19:04:45 UTC
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.
Comment 9 Christoph Reiter (lazka) 2015-01-20 11:21:44 UTC
I've opened bug 743062 for the leaks of GInitiallyUnowned subclasses in GStreamer.
Comment 10 André Klapper 2015-02-07 17:17:57 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Comment 11 Christoph Reiter (lazka) 2017-05-11 08:41:27 UTC
Since there has been some activity in gstreamer referencing this (bug 782499): The patch here still applies on master and passes tests.
Comment 12 Christoph Reiter (lazka) 2017-05-17 21:07:06 UTC
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.