Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal connections may cause leak of objects #150

Open
hugopl opened this issue Jun 10, 2024 · 0 comments · May be fixed by #152
Open

Signal connections may cause leak of objects #150

hugopl opened this issue Jun 10, 2024 · 0 comments · May be fixed by #152
Assignees
Labels
enhancement New feature or request

Comments

@hugopl
Copy link
Owner

hugopl commented Jun 10, 2024

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

@hugopl hugopl added the enhancement New feature or request label Jun 10, 2024
@hugopl hugopl self-assigned this Jun 10, 2024
hugopl added a commit that referenced this issue Jun 14, 2024
@hugopl hugopl linked a pull request Jun 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant