Fix missing GC.add_finalizer throughout the stdlib (#5367)

crystal-lang · Jan 2, 2018 · 0e48aac · 0e48aac
1 parent b549408
commit 0e48aac
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 0 deletions.
diff --git a/spec/std/object_spec.cr b/spec/std/object_spec.cr
@@ -1,4 +1,5 @@
 require "spec"
+require "../support/finalize"
 
 private class StringWrapper
   delegate downcase, to: @string
@@ -97,6 +98,18 @@ private class TestObject
   end
 end
 
+private class TestObjectWithFinalize
+  property key : Symbol?
+
+  def finalize
+    if key = self.key
+      State.inc(key)
+    end
+  end
+
+  def_clone
+end
+
 describe Object do
   describe "delegate" do
     it "delegates" do
@@ -337,4 +350,9 @@ describe Object do
   it "#unsafe_as" do
     0x12345678.unsafe_as(Tuple(UInt8, UInt8, UInt8, UInt8)).should eq({0x78, 0x56, 0x34, 0x12})
   end
+
+  it "calls #finalize on #clone'd objects" do
+    obj = TestObjectWithFinalize.new
+    assert_finalizes(:clone) { obj.clone }
+  end
 end
diff --git a/spec/std/openssl/ssl/context_spec.cr b/spec/std/openssl/ssl/context_spec.cr
@@ -1,5 +1,18 @@
 require "spec"
 require "openssl"
+require "../../../support/finalize"
+
+class OpenSSL::SSL::Context
+  property key : Symbol?
+
+  def finalize
+    if key = self.key
+      State.inc(key)
+    end
+
+    previous_def
+  end
+end
 
 describe OpenSSL::SSL::Context do
   it "new for client" do
@@ -147,4 +160,12 @@ describe OpenSSL::SSL::Context do
     context.alpn_protocol = "h2"
   end
   {% end %}
+
+  it "calls #finalize on insecure client context" do
+    assert_finalizes(:insecure_client_ctx) { OpenSSL::SSL::Context::Client.insecure }
+  end
+
+  it "calls #finalize on insecure server context" do
+    assert_finalizes(:insecure_server_ctx) { OpenSSL::SSL::Context::Server.insecure }
+  end
 end
diff --git a/spec/std/reference_spec.cr b/spec/std/reference_spec.cr
@@ -1,4 +1,5 @@
 require "spec"
+require "../support/finalize"
 
 private module ReferenceSpec
   class TestClass
@@ -35,6 +36,16 @@ private module ReferenceSpec
     def initialize(@x : Int32)
     end
   end
+
+  class TestClassWithFinalize
+    property key : Symbol?
+
+    def finalize
+      if key = self.key
+        State.inc(key)
+      end
+    end
+  end
 end
 
 describe "Reference" do
@@ -110,4 +121,9 @@ describe "Reference" do
     ReferenceSpec::TestClassBase.new.pretty_inspect.should match(/\A#<ReferenceSpec::TestClassBase:0x[0-9a-f]+>\Z/)
     ReferenceSpec::TestClass.new(42, "foo").pretty_inspect.should match(/\A#<ReferenceSpec::TestClass:0x[0-9a-f]+ @x=42, @y="foo">\Z/)
   end
+
+  it "calls #finalize on #dup'ed objects" do
+    obj = ReferenceSpec::TestClassWithFinalize.new
+    assert_finalizes(:clone) { obj.dup }
+  end
 end
diff --git a/spec/std/yaml/mapping_spec.cr b/spec/std/yaml/mapping_spec.cr
@@ -1,5 +1,6 @@
 require "spec"
 require "yaml"
+require "../../support/finalize"
 
 private class YAMLPerson
   YAML.mapping({
@@ -124,6 +125,20 @@ class YAMLRecursiveHash
   })
 end
 
+private class YAMLWithFinalize
+  YAML.mapping({
+    value: YAML::Any,
+  })
+
+  property key : Symbol?
+
+  def finalize
+    if key = self.key
+      State.inc(key)
+    end
+  end
+end
+
 describe "YAML mapping" do
   it "parses person" do
     person = YAMLPerson.from_yaml("---\nname: John\nage: 30\n")
@@ -445,4 +460,8 @@ describe "YAML mapping" do
       yaml.last_name_present?.should be_false
     end
   end
+
+  it "calls #finalize" do
+    assert_finalizes(:yaml) { YAMLWithFinalize.from_yaml("---\nvalue: 1\n") }
+  end
 end
diff --git a/spec/support/finalize.cr b/spec/support/finalize.cr
@@ -0,0 +1,29 @@
+class State
+  @@count = {} of Symbol => Int64
+
+  def self.inc(key)
+    @@count[key] = @@count.fetch(key, 0i64) + 1
+  end
+
+  def self.count(key)
+    @@count.fetch(key, 0i64)
+  end
+
+  def self.reset
+    @@count.clear
+  end
+end
+
+def assert_finalizes(key)
+  State.reset
+  State.count(key).should eq(0)
+
+  10.times do
+    obj = yield
+    obj.key = key
+  end
+
+  GC.collect
+
+  State.count(key).should be > 0
+end
diff --git a/src/object.cr b/src/object.cr
@@ -1182,6 +1182,7 @@ class Object
     def clone
       clone = \{{@type}}.allocate
       clone.initialize_copy(self)
+      GC.add_finalizer(clone) if clone.responds_to?(:finalize)
       clone
     end
 

diff --git a/src/openssl/ssl/context.cr b/src/openssl/ssl/context.cr
@@ -178,6 +178,7 @@ abstract class OpenSSL::SSL::Context
   protected def self.insecure(method : LibSSL::SSLMethod)
     obj = allocate
     obj._initialize_insecure(method)
+    GC.add_finalizer(obj)
     obj
   end
 

diff --git a/src/reference.cr b/src/reference.cr
@@ -46,6 +46,7 @@ class Reference
     {% else %}
       dup = self.class.allocate
       dup.as(Void*).copy_from(self.as(Void*), instance_sizeof(self))
+      GC.add_finalizer(dup) if dup.responds_to?(:finalize)
       dup
     {% end %}
   end

diff --git a/src/yaml/mapping.cr b/src/yaml/mapping.cr
@@ -106,6 +106,7 @@ module YAML
       ctx.record_anchor(node, instance)
 
       instance.initialize(ctx, node, nil)
+      GC.add_finalizer(instance) if instance.responds_to?(:finalize)
       instance
     end