I found myself doing a g_new()/g_free() to deal with this situation myself recently.  Patch welcome.

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.

Created attachment 256669 [details] [review]
Add g_weak_ref_new/free()

Heap allocated GWeakRef are useful to give as user_data on async calls.

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).

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?

I'm not doing any thread-safe code atm anyway.

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.

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);
}

(In reply to comment #8)
>   g_object_unref (self);
Actually without that line in that case.

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;
}

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).

-- 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.