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 698018 - Add an explicit g_binding_release()
Add an explicit g_binding_release()
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 699571
 
 
Reported: 2013-04-14 20:10 UTC by Emmanuele Bassi (:ebassi)
Modified: 2013-06-12 10:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
binding: Add an explicit release() (4.87 KB, patch)
2013-04-14 20:10 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
binding: Add an explicit release() (5.50 KB, patch)
2013-04-14 20:14 UTC, Emmanuele Bassi (:ebassi)
accepted-commit_now Details | Review
binding: Add an explicit unbind() (5.35 KB, patch)
2013-05-02 22:44 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
binding: Add an explicit unbind() (6.92 KB, patch)
2013-05-02 22:51 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
binding: Fix annotation of g_object_bind_property* (1.80 KB, patch)
2013-05-03 22:53 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
binding: Make unbind() release the reference on the binding (5.02 KB, patch)
2013-05-03 23:46 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
tests/binding: Ensure that the binding goes away (4.00 KB, patch)
2013-05-03 23:47 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Emmanuele Bassi (:ebassi) 2013-04-14 20:10:03 UTC
Languages with GC have issues releasing the last reference in a predictable way, so they need an explicit method for severing a binding between two properties.
Comment 1 Emmanuele Bassi (:ebassi) 2013-04-14 20:10:10 UTC
Created attachment 241523 [details] [review]
binding: Add an explicit release()

Higher order languages with garbage collection can have issues releasing
a binding, as they do not control the last reference being dropped on
the binding, source, or target instances.
Comment 2 Emmanuele Bassi (:ebassi) 2013-04-14 20:14:07 UTC
Created attachment 241524 [details] [review]
binding: Add an explicit release()

Higher order languages with garbage collection can have issues releasing
a binding, as they do not control the last reference being dropped on
the binding, source, or target instances.
Comment 3 Zeeshan Ali 2013-04-14 20:34:50 UTC
Review of attachment 241524 [details] [review]:

Looks good and I tested it against gir.

::: gobject/gbinding.c
@@ +761,3 @@
+ * This function does not change the reference count of @binding.
+ *
+ * This function is meant for language bindings.

More like meant for high-level languages themselves rather than bindings as this function needs to be explicitly called by apps.
Comment 4 Simon Feltman 2013-04-14 23:11:21 UTC
For the Python bindings I ended up returning a wrapper which weakly references the GBinding and has an explicit "unbind" method (which I think is a less ambiguous name than "release" FWIW).

https://git.gnome.org/browse/pygobject/tree/gi/_gobject/pygobject.c?h=pygobject-3-8#n2378
Comment 5 Emmanuele Bassi (:ebassi) 2013-05-02 22:44:58 UTC
Created attachment 243121 [details] [review]
binding: Add an explicit unbind()
Comment 6 Emmanuele Bassi (:ebassi) 2013-05-02 22:51:21 UTC
Created attachment 243124 [details] [review]
binding: Add an explicit unbind()

this time, with tests.
Comment 7 Emmanuele Bassi (:ebassi) 2013-05-02 22:52:10 UTC
Attachment 243124 [details] pushed as a360b31 - binding: Add an explicit unbind()
Comment 8 Simon Feltman 2013-05-03 09:41:06 UTC
This will make it easier for PyGObject to move more stuff to introspection, thanks!

Also the gobject-sections.txt file is using "g_binding_release" in the latest patch.
Comment 9 Simon Feltman 2013-05-03 19:27:22 UTC
When removing the static code from PyGObject (bug 699571), I ran into a problem where after calling unbind the internally managed reference of the GBinding is not released. Although this is the described behavior in the doc string:

"This function does not change the reference count of @binding."

This means high level languages will need to manually manage GBindings "self reference" in order to make things leaks free. For example:

Implicit cleanup case: Last ref held by language wrapper and will eventually be cleaned up with scoping/GC:
>> binding = a.bind_property('foo', b, 'foo')
>> binding.ref_count == 2
>> del a
>> binding.ref_count == 1

Explicit cleanup case: Two refs held after unbind, 1 for the language wrapper and 1 from the initial binding creation which is lost (ref leak)
>> binding = a.bind_property('foo', b, 'foo')
>> binding.unbind()
>> binding.ref_count == 2

I can solve this in PyGObject by overriding "unbind" and additionally calling "unref" manually, but it seems like unbind should handle this as it will also be a problem for other languages. Or am I missing something?
Comment 10 Emmanuele Bassi (:ebassi) 2013-05-03 20:44:03 UTC
the idea is that you keep the binding around in order to release it, and then remove it explicitly from your code — either by assigning the variable to another binding or assigning to None/null/whatever.

this is more explicit and maps to something that actually makes sense for every language without GLib knowing the semantics of the language you're using. for instance:

  this._binding = a.bind_property('foo', b, 'foo');
  ... // do stuff
  this._binding.release();
  this._binding = null; // or this._binding = a.bind_property('bar', b, 'bar');

yes, if you leave around this._binding then you leak an (inert) object, at least until the variable/instance that is holding it is collected.

I think making release() call unref() implicit would be a gross hack: it means basically providing an unref() method that is not subsumed by the language binding itself.

release() works in the same way as moving the release() implementation into GBinding.dispose, and letting high level languages call run_dispose(), without requiring an explicit method on GBinding.
Comment 11 Simon Feltman 2013-05-03 22:26:30 UTC
(In reply to comment #10)
> this is more explicit and maps to something that actually makes sense for every
> language without GLib knowing the semantics of the language you're using. for
> instance:
> 
>   this._binding = a.bind_property('foo', b, 'foo');
>   ... // do stuff
>   this._binding.release();
>   this._binding = null; // or this._binding = a.bind_property('bar', b, 'bar');
> 
> yes, if you leave around this._binding then you leak an (inert) object, at
> least until the variable/instance that is holding it is collected.

What I'm trying to express is that even if you clear "this._binding" out the GBinding will still leak. The reason for this is "a.bind_property(...)" will return an object with a ref count of 2. One held by the language and one held internally which is released when either the bindings source or target is released. So to make the above example not leak, you would need:

this._binding = a.bind_property('foo', b, 'foo');
... // do stuff
this._binding.unbind(); // does nothing to ref count
this._binding.unref();  // releases internally held ref
this._binding = null;   // releases ref held by language either immediately or when GC occurs

> I think making release() call unref() implicit would be a gross hack: it means
> basically providing an unref() method that is not subsumed by the language
> binding itself.

The proposed unref() call within unbind() doesn't have anything to do with the language binding ref, but is for managing the ref held internally by the GBinding itself, similar to how it is managed with weak refs to the binding source or target.

The language binding will handle referencing normally because "g_object_bind_property" is transfer-none, so languages will add their own ref which they manage.

I don't think Gjs is that much different from PyGObject in terms of GObject reference count management. So I'm also assuming it is going to suffer the same leak I'm talking about, but I cannot easily verify this because I don't see the GObject.ref_count field available in Gjs.
Comment 12 Emmanuele Bassi (:ebassi) 2013-05-03 22:41:05 UTC
there isn't anything in GObject that adds a reference to the GBinding instance returned by g_object_bind_property(): the instance is created with an initial reference count of 1, like any object returned by g_object_new(). if unbind() called unref(), then the instance would go away from a C perspective, which is something that I don't want.

the memory management of the binding instance is implicit at the C level: the GBinding object exists only as long as the binding exists.

should we change annotation to g_object_bind_property() and friends, and mark them as (transfer full) instead of (transfer none), if this solves the reference counting from the PyGObject perspective?

(as a side note: I think this whole "let's check the ref_count field of GObject from Python" is completely wrong; not even C developers are supposed to check the ref_count of a GObject without a ton of caveats).
Comment 13 Emmanuele Bassi (:ebassi) 2013-05-03 22:53:31 UTC
Created attachment 243256 [details] [review]
binding: Fix annotation of g_object_bind_property*

The returned GBinding instance is constructed by the functions.
Comment 14 Simon Feltman 2013-05-03 23:04:42 UTC
(In reply to comment #12)
> there isn't anything in GObject that adds a reference to the GBinding instance
> returned by g_object_bind_property(): the instance is created with an initial
> reference count of 1, like any object returned by g_object_new(). if unbind()
> called unref(), then the instance would go away from a C perspective, which is
> something that I don't want.

I don't see it this way, g_object_new is transfer full so the caller is expected to fully manage the ref returned. In the case of g_object_bind_property being transfer none, to me means in essence, it is returning a "borrowed reference", that of which is being internally managed by the GBinding itself based on source and target. So language bindings need to add an additional ref so we don't decref this internal borrowed ref.

> the memory management of the binding instance is implicit at the C level: the
> GBinding object exists only as long as the binding exists.

We also want the ability for implicit management in higher level languages as this is one of the main selling points of GBinding in practice.

> should we change annotation to g_object_bind_property() and friends, and mark
> them as (transfer full) instead of (transfer none), if this solves the
> reference counting from the PyGObject perspective?

No, this would break the implicit ref management of GBinding because something like:

>>> a.bind_property('foo', b, 'foo', ...)

Would create the binding and immediately destroy it. I think having this work is actually the more useful case in the practice of building dynamic UIs. I don't want to keep lists of binding objects around if I don't have to. Just destroying the dynamically generated UI should clean up the GBinding objects automatically.

> (as a side note: I think this whole "let's check the ref_count field of GObject
> from Python" is completely wrong; not even C developers are supposed to check
> the ref_count of a GObject without a ton of caveats).

I'm curious why you think it's wrong, and I agree that in production code it is, but I've been able to find an fix an enormous amount of reference leaks in PyGObject by writing unittest based on expected results of the GObject ref count.

(In reply to comment #13)
> Created an attachment (id=243256) [details] [review]
> binding: Fix annotation of g_object_bind_property*
> 
> The returned GBinding instance is constructed by the functions.

Please, don't do this! as mentioned above it will break things :)
Comment 15 Emmanuele Bassi (:ebassi) 2013-05-03 23:46:07 UTC
Created attachment 243261 [details] [review]
binding: Make unbind() release the reference on the binding

Hopefully, people will read the note before filing bugs because
g_binding_unbind() makes the GBinding instance invalid.
Comment 16 Emmanuele Bassi (:ebassi) 2013-05-03 23:47:00 UTC
Created attachment 243262 [details] [review]
tests/binding: Ensure that the binding goes away

Use weak pointers so that we can check that the GBinding instance goes
away when it should.
Comment 17 Simon Feltman 2013-05-04 00:01:50 UTC
Review of attachment 243261 [details] [review]:

Looks good to me, although I don't know what the hash table changes are for.
Also I think the gobject-sections.txt needs to be updated from "g_binding_release" to "g_binding_unbind"?
Comment 18 Simon Feltman 2013-05-04 00:03:52 UTC
Review of attachment 243262 [details] [review]:

Looks good.
Comment 19 Emmanuele Bassi (:ebassi) 2013-05-04 00:10:48 UTC
(In reply to comment #17)
> Review of attachment 243261 [details] [review]:
> 
> Looks good to me, although I don't know what the hash table changes are for.

churn than ended up in a git rebase; I'll split it off into its own commit prior to pushing.

> Also I think the gobject-sections.txt needs to be updated from
> "g_binding_release" to "g_binding_unbind"?

yes, it's inside another commit that I didn't put on bugzilla, since it's trivial.
Comment 20 Simon Feltman 2013-05-04 01:17:00 UTC
Re-opening so it can stay as a valid dependency of bug 699571.
Comment 21 Simon Feltman 2013-05-04 15:24:07 UTC
Review of attachment 243261 [details] [review]:

::: gobject/gbinding.c
@@ +813,2 @@
 {
   g_return_if_fail (G_IS_BINDING (binding));

Additionally, I think we should add checks for both source and target here. If either one are NULL then fill out a GError stating the binding has already been unbound (by either explicit or implicit means). This will block against the unref being called in an invalid manure, the source or target being finalized or someone calling "unbind" more than once.
Comment 22 Simon Feltman 2013-05-04 16:27:04 UTC
(In reply to comment #21)
> Additionally, I think we should add checks for both source and target here. If
> either one are NULL then fill out a GError stating the binding has already been
> unbound (by either explicit or implicit means).

I can help work out a patch for the above.
Comment 23 Emmanuele Bassi (:ebassi) 2013-05-04 16:34:34 UTC
(In reply to comment #21)
> Review of attachment 243261 [details] [review]:
> 
> ::: gobject/gbinding.c
> @@ +813,2 @@
>  {
>    g_return_if_fail (G_IS_BINDING (binding));
> 
> Additionally, I think we should add checks for both source and target here. If
> either one are NULL then fill out a GError stating the binding has already been
> unbound (by either explicit or implicit means). This will block against the
> unref being called in an invalid manure, the source or target being finalized
> or someone calling "unbind" more than once.

no, *absolutely* not: this is a programmer error, and GError is not for that class of errors.
Comment 24 Simon Feltman 2013-05-04 17:34:46 UTC
(In reply to comment #23)
> no, *absolutely* not: this is a programmer error, and GError is not for that
> class of errors.

In this case I would nix the whole unref in unbind. The issue is the potential for either source or target being finalized which would cause the GBinding to also be unrefed, a follow up unbind call would then unref the language bindings held ref. Perhaps this is similar to what you have been concerned about the whole time? 

The root of the problem is that we are trying to allow two modes of operation for a property binding with the same API. This could be broken down as being explicit methods which essentially spell out their behavior:

 void      g_object_bind_property_weak (...)
 GBinding *g_object_bind_property_full (...) // (transfer-full)

Note the void return in the weak version, meaning you have no way to even attempt manual management of the binding and cleanup only occurs implicitly based on the lifetime of source or target. But this breaks API with changing the manually managed version to transfer-full.

Given a lack of safety with unref in unbind and no ability to change the API, it seems the best we can probably do is to have a leak in some languages which want to explicitly use unbind. I can fix the ref leak on the PyGObject side of things with what has already been committed, things will however leak in other language bindings unless they are also fixed with some type of override.
Comment 25 Simon Feltman 2013-05-04 21:10:20 UTC
(In reply to comment #24)
> (In reply to comment #23)
> > no, *absolutely* not: this is a programmer error, and GError is not for that
> > class of errors.
> 
> In this case I would nix the whole unref in unbind.

Perhaps I was a little to quick with this assessment. Would g_return_if_fail work for you instead of a GError?

g_return_if_fail (binding->source != NULL);
g_return_if_fail (binding->target != NULL);

Given this with an unref in unbind, it should fix the leak and also block against potential crashes after an invalid unbind is used.

The reason I initially suggested GError is because it is better for high level languages because it will translate into an exception.
Comment 26 Emmanuele Bassi (:ebassi) 2013-05-16 21:39:37 UTC
(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)

> Given this with an unref in unbind, it should fix the leak and also block
> against potential crashes after an invalid unbind is used.

if unbind() is called and something in the language binding is still holding a reference, then the binding instance will still be valid but inert (source and target will be NULL). calling unbind() again on it will not do anything: everything has to be NULL-safe internally, because the unbind() code has to be called in two places already — the weakref notification of either source or target, and the binding's finalize.
Comment 27 Emmanuele Bassi (:ebassi) 2013-06-12 10:30:47 UTC
let's close this with the new behaviour of unbind() that was agreed upon, before we need to freeze the API

I've pushed something similar to attachment 243261 [details] [review], without the hash table chunks.

Attachment 243262 [details] pushed as 910732e - tests/binding: Ensure that the binding goes away