hugopl/gi-crystal

Signal connections may cause leak of objects

Opened this issue · 0 comments

GI-Crystal tries to let signal connections code feel like any other Crystal code, you can use blocks, closures, whatever, then the connect method will return an signal handler id that you must use to disconnect the signal, surprise... nobody does that.

In C there's no need to disconnect all signals before destroying an object, this will be done anyway, however in GI-Crystal things are different:

To support closures in signal connections it needs to store this closure somewhere to avoid the garbage collector to collect it, so there's a GICrystal::ClosureDataManager singleton that does that, the problem? The closure will probably also have a reference to the object you used in the signal connection, so the GC will never collect the GObject because there's a reference on it in the closure stored in the ClosureDataManager, so unless you disconnect all signals that the GC will never collect your beloved GObject.

I'm aware of this for years, but last day I was playing with a new way to connect signals without need of a ClosureDataManager, of course this means it wont be possible to use closures on signal connections 😬.

The idea is to have a connect macro method in GObject, so instead of doing:

button.signal.connect(->on_button_clicked)

you do

connect(button.signal, on_button_clicked)

the macro creates a non-closure proc that receives a boxed WeakRef as signal user_data and uses it to call the signal handler.

The WeakRef is stored in the ClosureDataManager to avoid being collected by the GC, so the GObject can be collected by the GC since there will be no reference for it on ClosureDataManager.

The code bellow is just a very very very rough draft that I'll paste here just to not loss it between today and the day I'll implement this.

  macro connect(signal, handler)
    %box = ::Box.box(WeakRef.new(self))
    %sig = {{ signal }}

    %slot = ->(sender : Void*,
        {% i = 0 %}
        {% for arg in handler.args %}
        {% resolved_type = arg.resolve %}
          arg_{{ i }} : {{ arg }},
        {% end %}
        box : Void*) {
      ref = ::Box(WeakRef({{ @type }})).unbox(box).value
      ref.{{ handler.name }}(
      {% for i in 0...handler.args.size %}
        arg_{{i}},
      {% end %}
      ) if ref
      nil
    }
pp! %slot.closure?
    LibGObject.g_signal_connect_data(%sig.source, %sig.name, %slot.pointer,
      GICrystal::ClosureDataManager.register(%box),
      ->GICrystal::ClosureDataManager.deregister, 0_u32)

  end

This macro depends on nothing of GObject, maybe I kept it on GObject namespace, so will be possible to use it on non-gobject objects. The only downside is that will not be possible to connect signals to top level functions, but this can be fixed as well.

If I implement this... at first I wont deprecate the current .connect methods on Signal objects, but once I feel that this new way is working ok I will probably deprecated them