Skip to content

Commit

Permalink
Implement a memory safe way to connect signals.
Browse files Browse the repository at this point in the history
Fixes #150
  • Loading branch information
hugopl committed Jun 14, 2024
1 parent 9d9aced commit 12749f5
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 2 deletions.
3 changes: 3 additions & 0 deletions ecr/signal.ecr
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ struct <%= signal_type %> < GObject::Signal
@detail ? "<%= signal.name %>::#{@detail}" : "<%= signal.name %>"
end

def validate_params(<%= validation_args %>)
end

def connect(*, after : Bool = false, &block : Proc(<%= lean_proc_params %>)) : GObject::SignalConnection
connect(block, after: after)
end
Expand Down
68 changes: 68 additions & 0 deletions spec/safe_signal_spec.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
require "./spec_helper"

private class Obj < GObject::Object
getter int32 = 0
getter bool = false

signal int32_bool(int32 : Int32, bool : Bool)

def initialize(int32_value : Int32, bool_value : Bool)
super()
# Uncomment this and the test will fail, because a reference is left on ClosureManager
#
# int32_bool_signal.connect do |int, bool|
# int32_bool_slot(int, bool)
# end

GObject.connect(int32_bool_signal, int32_bool_slot(Int32, Bool))
int32_bool_signal.emit(int32_value, bool_value)
end

def int32_bool_slot(@int32, @bool)
end
end

@[NoInline]
private def create_obj(int32_value : Int32, bool_value : Bool)
obj = Obj.new(int32_value, bool_value)
{WeakRef.new(obj), obj.int32, obj.bool}
end

private class Subject < Test::Subject
getter enum : Test::RegularEnum = Test::RegularEnum::Value1

def initialize(enum_value : Test::RegularEnum)
super()
GObject.connect(enum_signal, enum_slot(Test::RegularEnum))
enum_signal.emit(enum_value)
end

def enum_slot(@enum)
end
end

@[NoInline]
private def create_subject(enum_value)
obj = Subject.new(enum_value)
{WeakRef.new(obj), obj.enum}
end

describe "Save signals" do
it "does not leak memory" do
obj_ref, int32, bool = create_obj(int32_value: 42_u32, bool_value: true)
int32.should eq(42)
bool.should eq(true)
# Need to call some obj_ref method to avoid LLVM optimizations that break this test.
obj_ref.inspect
GC.collect
obj_ref.value.should eq(nil)
end

it "works for non-user signals the same way" do
obj_ref, enum_ = create_subject(enum_value: Test::RegularEnum::Value2)
enum_.should eq(Test::RegularEnum::Value2)
obj_ref.inspect
GC.collect
obj_ref.value.should eq(nil)
end
end
1 change: 1 addition & 0 deletions src/bindings/g_object/binding.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ require_after:
- value.cr
- signal_connection.cr
- gi_crystal.cr
- connect.cr

ignore_constants:
- PARAM_MASK
Expand Down
58 changes: 58 additions & 0 deletions src/bindings/g_object/connect.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
require "weak_ref"

module GObject
@[Experimental("Join discussion about this topic at https://github.com/hugopl/gi-crystal/issues/150")]
macro connect(signal, handler)
begin
%box = ::Box.box(WeakRef.new(self))
%sig = {{ signal }}
%sig.validate_params({{ handler.args.splat }})

# Declare "C" slot
%c_slot = ->(sender : Void*,
{% i = 0 %}
{% for arg in handler.args %}
{%
resolved_type = arg.resolve
if resolved_type == String || resolved_type == Path
type = ::Pointer(UInt8)
elsif resolved_type == Bool
type = ::Int32
else
type = resolved_type
end
%}
c_arg_{{ i }} : {{ type }},
{% i += 1 %}
{% end %}
box : Void*) {

ref = ::Box(WeakRef(typeof(self))).unbox(box).value || raise GICrystal::ObjectCollectedError.new

# Convert from C
{% i = 0 %}
{% for arg in handler.args %}
{% resolved_type = arg.resolve %}
{% if resolved_type == String %}
arg_{{ i }} = String.new(c_arg_{{ i }})
{% elsif resolved_type == Path %}
arg_{{ i }} = Path.new(String.new(c_arg_{{ i }})
{% elsif resolved_type == Bool %}
arg_{{ i }} = GICrystal.to_bool(c_arg_{{ i }})
{% else %}
arg_{{ i }} = c_arg_{{ i }}
{% end %}
{% i += 1 %}
{% end %}

ref.{{ handler.name }}( {{ (0...handler.args.size).map { |i| "arg_#{i}".id }.splat }} )
nil
}
sig_handler_id = LibGObject.g_signal_connect_data(%sig.source, %sig.name, %c_slot.pointer,
GICrystal::ClosureDataManager.register(%box),
->GICrystal::ClosureDataManager.deregister, 0_u32)

GObject::SignalConnection.new(%sig.source, sig_handler_id)
end
end
end
7 changes: 7 additions & 0 deletions src/bindings/g_object/object.cr
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,13 @@ module GObject

alias LeanProc = Proc({{ (signature.args.map(&.type) << Nil).splat }})

def validate_params(
{% for arg in signature.args %}
{{ arg }}.class,
{% end %}
)
end

def connect(*, after : Bool = false, &block : LeanProc) : GObject::SignalConnection
connect(block, after: after)
end
Expand Down
4 changes: 2 additions & 2 deletions src/bindings/g_object/signal.cr
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ require "./signal_connection"

module GObject
abstract struct Signal
@source : GObject::Object
@detail : String?
getter source : GObject::Object
getter detail : String?

def initialize(@source, @detail = nil)
end
Expand Down
15 changes: 15 additions & 0 deletions src/generator/signal_gen.cr
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,21 @@ module Generator
end
end

private def validation_args : String
String.build do |s|
@args_strategies.each do |arg_strategy|
next if arg_strategy.remove_from_declaration?

arg = arg_strategy.arg
arg_type_info = arg.type_info
nullmark = " | Nil" if arg.nullable?

s << to_identifier(arg.name) << " : "
s << to_crystal_type(arg_type_info, include_namespace: true) << ".class" << nullmark << ','
end
end
end

private def lean_proc_params : String
String.build do |s|
arg_strategies_to_proc_param_string(s, @signal, @args_strategies)
Expand Down

0 comments on commit 12749f5

Please sign in to comment.