hugopl/gi-crystal

Support GC resistant GObject subclasses

hugopl opened this issue · 2 comments

hugopl commented

When passing a instance of a object that inherits GObject to a function that takes the ownershipt of this object, if the only Crystal instance dies, the GC collects the memory, so later when you retrieve the object and try to reding it to a Crystal object an exception is thrown (or a crash happen somewhere).

Among many awesome patches, @BlobCodes fixed this issue at #64, however the approach used IMO changes the common way of doing things in Crystal too much, so I choose to not merge it until we find a better solution.

To make things clear, here's a spec test that reproduce the issue.

require "./spec_helper"

private class GCResistantObj < GObject::Object
  property moto : String

  def initialize(@moto)
    super()
  end
end

@[NoInline]
def feed_gc : Test::Subject
  subject = Test::Subject.new
  gc_resistance = GCResistantObj.new("viva la resistance!")
  subject.gobj = gc_resistance
  subject
end

describe "GC friendness" do
  it "does not collect objects that goes to C world" do

    subject = feed_gc
    GC.collect
    # Nowadays this raises GICrystal::ObjectCollectedError, because the object was collected.
    hey = GCResistantObj.cast(subject.gobj)
    # Nowadays this fails with TypeCastError because it's accessing memory garbage collected
    # hey = subject.gobj.as(GCResistantObj)
    hey.moto.should eq("viva la resistance!")
  end
end

Copy this under spec/, run make tests, then later ./bin/spec spec/gc_spec.cr -Ddebugmemory to better see things happening.

@hugopl

Among many awesome patches, @BlobCodes fixed this issue at #64, however the approach used IMO changes the common way of doing things in Crystal too much, so I choose to not merge it until we find a better solution.

I think that this is not the case (at least regarding the current issue).
I think my design choices were not understood, so I'll explain why the current PR is the way it is, what other options we have, and what we should consider moving forward.

The problem

A crystal-based GObject subclass needs its data from the c world, so the refcount of the GObject will always be >= 1.
But since the GObject could be used only from the c world, we cannot allow the GC to free the crystal object once there are no references to it.
This results in a deadlock: The refcount will always be >=1, but there must also always be a crystal object.
This issue is not easy to solve, but we have options.

The old PR (#28)

My previous PR used two memory allocations.
There was a private allocation for all the data required inside the GObject, which was only freed once the GObject itself was destroyed.
Then, there was a public allocation to be used in the crystal code, which was structured just like all the other GObject bindings (only a pointer to the c struct).
This all worked and was quite performant, but it resulted in messy code. Either the used had to define two classes, or huge macros needed to be used.

The current PR (#64)

The current PR uses a toggle ref.
The toggle ref is a concept from GLib, only created to support GC languages.
It allows to create callbacks for the following two cases:

  • The refcount dropped to exactly 1
  • The refcount increased to >1
    This allows us to place a pointer to our crystal object in a hashmap (or similar) once there are external references to it, ensuring it will not be collected. But once there are no references left from the c-world, we can remove this pointer so the crystal object can be garbage collected once there also aren't any crystal references left.

So the way the current PR solves the deadlock is probably the best possible way, because it is the only option backed by GLib - and we do not really have any useful tricks in crystal-world unless the compiler is modified.

Problems with the current PR

The reason the current PR is not very crystal-ish is because it also tries to solve a load of other problems.

  • The PR also allows crystal-based GObject subclasses to be created outside of crystal, for example using blueprint or the GtkBuilder XML.

    The reason this feature is also in the PR is because this was the primary reason why I wanted GC-resistancy.

  • The PR also allows to pass any other properties inside the constructor).

    The GObject type system allows having construct-only properties, like Gtk::Widget.css-name.
    I wanted to set one of those properties from crystal in my app, so I made the PR automatically generate a constructor similar to the bindings:

    class MyWidget < Gtk::Widget
      # If you have a constructor like this:
      def initialize(*, @a : Int32, @my_long_variable_name : String, &)
      end
    
      # my PR generated something like this:
      def self.new(*, a : Int32? = nil, my_long_variable_name : String? = nil, accessible_role : Gtk::AccessibleRole? = nil, can_focus : Bool? = nil, can_target : Bool? = nil, css_classes : Enumerable(String)? = nil, css_name : String? = nil, cursor : Gdk::Cursor? = nil, focus_on_click : Bool? = nil, focusable : Bool? = nil, halign : Gtk::Align? = nil, has_default : Bool? = nil, has_focus : Bool? = nil, has_tooltip : Bool? = nil, height_request : Int32? = nil, hexpand : Bool? = nil, hexpand_set : Bool? = nil, layout_manager : Gtk::LayoutManager? = nil, margin_bottom : Int32? = nil, margin_end : Int32? = nil, margin_start : Int32? = nil, margin_top : Int32? = nil, name : String? = nil, opacity : Float64? = nil, overflow : Gtk::Overflow? = nil, parent : Gtk::Widget? = nil, receives_default : Bool? = nil, root : Gtk::Root? = nil, scale_factor : Int32? = nil, sensitive : Bool? = nil, tooltip_markup : String? = nil, tooltip_text : String? = nil, valign : Gtk::Align? = nil, vexpand : Bool? = nil, vexpand_set : Bool? = nil, visible : Bool? = nil, width_request : Int32? = nil)
        # NOTE: Content not relevant
      end

This is what created such specific requirements when creating GObject subclasses.

Conclusion

I think that we should definitely add ToggleRef-based GC-resistancy to the classes (that shouldn't even be complicated or cause any breaking changes).
I also think that we need to find solutions for the other problems listed above.
But maybe we can find better designs to solve these problems, one problem at a time.

hugopl commented

Yeah, I had understood your design and knew about ToggleRef from your PR 😁. Even the problems begin related, I think we must solve them separately, or at least in steps... first GC resistant objects, the easy one, then let Crystal objects born in C world.

For this bug in specific the solution is clear with ToggleRef, for the "Born in C Crystal objects" issue I have some ideas and need to write some POCs yet, but I'll create a separate issue so we can discuss things there.