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 680813 - Consider adding g_weak_ref_new/free()
Consider adding g_weak_ref_new/free()
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gobject
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-07-30 08:15 UTC by Xavier Claessens
Modified: 2018-05-24 14:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add g_weak_ref_new/free() (2.42 KB, patch)
2013-10-07 19:54 UTC, Xavier Claessens
needs-work Details | Review

Description Xavier Claessens 2012-07-30 08:15:40 UTC
I think it is a common use case to have a GObject as user_data of an async call, and you can't always cancel the call when the object dies. You don't always want to make the object survive until the operation finish.

I would like to do something like that:

my_callback (GObject *source,
    GAsyncResult *result,
    gpointer user_data)
{
  GWeakRef *weak = user_data;
  MyFoo *self = g_weak_ref_get (weak);

  if (self == NULL)
    goto out;

  ...

  g_object_unref (self);

out:
  g_weak_ref_free (weak);
}

my_foo_async (self->priv->something, my_callback, g_weak_ref_new (self));


We have TpWeakRef in telepathy-glib library for exacly that usecase. It has the convenience to have an extra gpointer user_data to be able to pass 2 stuff to the callback.

Or maybe we could add a new typedef struct {GWeakRef weak; gpointer user_data; GDestroyNotify destroy;} GWeakData; ?
Comment 1 Allison Karlitskaya (desrt) 2012-08-01 07:56:46 UTC
I found myself doing a g_new()/g_free() to deal with this situation myself recently.  Patch welcome.
Comment 2 Allison Karlitskaya (desrt) 2012-08-01 07:59:10 UTC
Interesting thought: the original version of GWeakRef was based on a refcounted auxiliary object that all weakref users held a reference on and which in turn contained something pretty much equivalent to the existing GWeakRef implementation.

It would be interesting to consider doing that for the API you are suggesting adding.  At the very least we should ensure that the added API makes switching to this style possible in the future.
Comment 3 Xavier Claessens 2013-10-07 19:54:31 UTC
Created attachment 256669 [details] [review]
Add g_weak_ref_new/free()

Heap allocated GWeakRef are useful to give as user_data on async calls.
Comment 4 Allison Karlitskaya (desrt) 2013-10-09 13:31:56 UTC
Review of attachment 256669 [details] [review]:

This lacks the refcounting that I would have liked to have seen.

Consider GVariantBuilder.  It has this API:

_init
_clear
_new
_ref
_unref

but it is very clearly documented that you absolutely must not call _ref or _unref on stack-allocated (read _init'd) builders.

I think we want the same here.  If you use _new() then you get the ability to _ref() and _unref(), otherwise you must use _clear().

A bit of explanation: I am specifically interested in solving the case of accessing a weak ref from one thread and dispatching to another thread for handling (where the object may die before the hander arrives in the other thread).  I want to do this without taking a hard ref on the object because this may result in the toggle ref handler being dispatched from a 'bad thread', which we've seen to cause problems with some language bindings (like pygobject, bug 709223) and also because this is usually done for the purpose of firing a signal and if the object is dead once the signal arrives in the destination thread, it's far better not to bother dispatching it.

This means that I need a way to take one weak ref and get another weak ref out of it in a safe way that does not involving taking a strong ref.  The only two options are g_weak_ref_ref() or some sort of copy-constructor g_weak_ref_copy() that operates under the lock.  Of these two I prefer the first (the ref).
Comment 5 Allison Karlitskaya (desrt) 2013-10-15 13:40:58 UTC
After some consideration, I consider the purpose for which I wanted this API to be an anti-pattern.  I intend now to create a non-threadsafe refcounted weakref.  Your original bug looks single-threaded -- would this work for you?
Comment 6 Xavier Claessens 2013-10-15 14:44:10 UTC
I'm not doing any thread-safe code atm anyway.
Comment 7 Allison Karlitskaya (desrt) 2013-10-25 13:20:27 UTC
Going to close this WONTFIX for now then.

We didn't deprecate the old weakrefs because they are still totally valid for non-threadsafe uses and they are more efficient.

For the record: the main intended use case of GWeakRef is for a statically-allocated singleton-holder that is accessed by multiple threads.  GWeakRef is not intended to be used in non-multithreaded situations.
Comment 8 Xavier Claessens 2014-04-07 18:02:47 UTC
GWeakRef is designed to be thread-safe, but that doesn't mean it can't be useful even in non-threaded code. I still believe that my initial use case is valid, and we don't have currently any easy way to do it.

I still think that adding those function can be immensely useful:
g_weak_ref_new()
g_weak_ref_ref()
g_weak_ref_unref()

If we don't want to use GWeakRef in that case, the example code would look like much more ugly IMO:

my_callback (GObject *source,
    GAsyncResult *result,
    gpointer user_data)
{
  MyFoo **self_ptr = user_data;
  MyFoo *self = *self_ptr;

  if (self == NULL)
    goto out;

  ...

  g_object_unref (self);

out:
  g_free (self_ptr);
}

void
my_foo_func (MyFoo *self)
{
  MyFoo **self_ptr = g_new (MyFoo*);
  *self_ptr = self;
  g_object_add_weak_pointer (G_OBJECT (self), self_ptr);
  my_foo_async (self->priv->something, my_callback, self_ptr);
}
Comment 9 Xavier Claessens 2014-04-07 18:04:56 UTC
(In reply to comment #8)
>   g_object_unref (self);
Actually without that line in that case.
Comment 10 Allison Karlitskaya (desrt) 2014-04-07 19:44:53 UTC
Seems like what you really want is this this:

void *g_object_get_weak_pointer (obj) {
  void *ptr = new();
  ptr = obj;
  obj.add_weak (&ptr);

  return ptr;
}
Comment 11 Allison Karlitskaya (desrt) 2016-05-18 15:30:48 UTC
It seems the xclaesse has a new usecase that requires thread safety after all..

Here's the plan:

Add a new GHeapWeakRef structure which is defined privately in gobject.c and contains a refcount and a magic number field.

Add _new() which allocates this, sets refcount to 1, fills in a magic number, and calls _init().  Add _ref() and _unref() that manipulate the refcount, only after verifying that the magic number is correct (to prevent use of _ref() and _unref() on static-allocated GWeakRef, same as we do for GVariantBuilders).


I would also like to add a g_object_get_weak_ref() which will return a common GWeakRef which is owned by the object and is shared by all callers to this API.  There was some argument during the initial design of GWeakRef that this arrangement would have been more useful for some cases.  The problem here is that we would need to prevent _set() and _init() from being called on this shared weakref.  We could use a separate magic number to enforce that, but _set() and _init() are expected to work on non-heap-allocated weakrefs as well, which don't have the magic number field, so we can't do that.  _init(), in particular, is expected to work on any piece of uninitialised memory.  There are still a few possible ways to enforce this, though.  When using _set() on a GWeakRef with an existing object, we could check that object to make sure we are not operating on its shared weak ref [slowish].  I think a reasonable compromise would be to check at each call to _get_weak_ref() and at object destruction, that the correct object is still in place and throw a critical if not.  It means that we would catch the problem, but not at a point where 'bt' would help us... oh well...


That means that we'd end up with something like this for most new users:


 GWeakRef *foo = g_object_get_weak_ref(obj);

  ...

 g_weak_ref_unref (foo);


and of course, we would also add support for g_autoptr(GWeakRef).
Comment 12 GNOME Infrastructure Team 2018-05-24 14:23:36 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/578.