Warn on a finalizer that captures the object to be finalized

Also improve specs and documentation for finalizers and more clearly
recommend a safe code pattern to use them.

gc.c

@@ -3384,6 +3384,57 @@ should_be_finalizable(VALUE obj)
  *  as an argument to <i>aProc</i>. If <i>aProc</i> is a lambda or
  *  method, make sure it can be called with a single argument.
  *
+ *  The return value is an array <code>[0, aProc]</code>.
+ *
+ *  The two recommended patterns are to either create the finaliser proc
+ *  in a non-instance method where it can safely capture the needed state,
+ *  or to use a custom callable object that stores the needed state
+ *  explicitly as instance variables.
+ *
+ *      class Foo
+ *        def initialize(data_needed_for_finalization)
+ *          ObjectSpace.define_finalizer(self, self.class.create_finalizer(data_needed_for_finalization))
+ *        end
+ *      
+ *        def self.create_finalizer(data_needed_for_finalization)
+ *          proc {
+ *            puts "finalizing #{data_needed_for_finalization}"
+ *          }
+ *        end
+ *      end
+ *
+ *      class Bar
+ *       class Remover
+ *          def initialize(data_needed_for_finalization)
+ *            @data_needed_for_finalization = data_needed_for_finalization
+ *          end
+ *        
+ *          def call(id)
+ *            puts "finalizing #{@data_needed_for_finalization}"
+ *          end
+ *        end
+ *
+ *        def initialize(data_needed_for_finalization)
+ *          ObjectSpace.define_finalizer(self, Remover.new(data_needed_for_finalization))
+ *        end
+ *      end
+ *
+ *  Note that if your finalizer references the object to be
+ *  finalized it will never be run on GC, although it will still be
+ *  run at exit. You will get a warning if you capture the object
+ *  to be finalized as the receiver of the finalizer.
+ *
+ *      class CapturesSelf
+ *        def initialize(name)
+ *          ObjectSpace.define_finalizer(self, proc {
+ *            # this finalizer will only be run on exit
+ *            puts "finalizing #{name}"
+ *          })
+ *        end
+ *      end
+ *
+ *  Also note that finalization can be unpredictable and is never guaranteed
+ *  to be run except on exit.
  */
 
 static VALUE
@@ -3400,6 +3451,10 @@ define_final(int argc, VALUE *argv, VALUE os)
 	should_be_callable(block);
     }
 
+    if (rb_callable_receiver(block) == obj) {
+        rb_warn("finalizer references object to be finalized");
+    }
+
     return define_final0(obj, block);
 }
 
@@ -12101,9 +12156,9 @@ rb_gcdebug_remove_stress_to_class(int argc, VALUE *argv, VALUE self)
  *
  *  ObjectSpace also provides support for object finalizers, procs that will be
  *  called when a specific object is about to be destroyed by garbage
- *  collection.
- *
- *     require 'objspace'
+ *  collection. See the documentation for
+ *  <code>ObjectSpace.define_finalizer</code> for important information on
+ *  how to use this method correctly.
  *
  *     a = "A"
  *     b = "B"
@@ -12111,6 +12166,9 @@ rb_gcdebug_remove_stress_to_class(int argc, VALUE *argv, VALUE self)
  *     ObjectSpace.define_finalizer(a, proc {|id| puts "Finalizer one on #{id}" })
  *     ObjectSpace.define_finalizer(b, proc {|id| puts "Finalizer two on #{id}" })
  *
+ *     a = nil
+ *     b = nil
+ *
  *  _produces:_
  *
  *     Finalizer two on 537763470

include/ruby/internal/intern/proc.h

@@ -46,6 +46,7 @@ VALUE rb_method_call_with_block(int, const VALUE *, VALUE, VALUE);
 VALUE rb_method_call_with_block_kw(int, const VALUE *, VALUE, VALUE, int);
 int rb_mod_method_arity(VALUE, ID);
 int rb_obj_method_arity(VALUE, ID);
+VALUE rb_callable_receiver(VALUE);
 VALUE rb_protect(VALUE (*)(VALUE), VALUE, int*);
 
 RBIMPL_SYMBOL_EXPORT_END()

proc.c

@@ -2739,6 +2739,18 @@ rb_obj_method_arity(VALUE obj, ID id)
     return rb_mod_method_arity(CLASS_OF(obj), id);
 }
 
+VALUE
+rb_callable_receiver(VALUE callable) {
+    if (rb_obj_is_proc(callable)) {
+        VALUE binding = rb_funcall(callable, rb_intern("binding"), 0);
+        return rb_funcall(binding, rb_intern("receiver"), 0);
+    } else if (rb_obj_is_method(callable)) {
+        return method_receiver(callable);
+    } else {
+        return Qundef;
+    }
+}
+
 const rb_method_definition_t *
 rb_method_def(VALUE method)
 {

spec/ruby/core/objectspace/define_finalizer_spec.rb

@@ -1,24 +1,42 @@
 require_relative '../../spec_helper'
 require_relative 'fixtures/classes'
 
-# NOTE: A call to define_finalizer does not guarantee that the
-# passed proc or callable will be called at any particular time.
+# Why do we not test that finalizers are run by the GC? The documentation
+# says that finalizers are never guaranteed to be run, so we can't
+# spec that they are. On some implementations of Ruby the finalizers may
+# run asyncronously, meaning that we can't predict when they'll run,
+# even if they were guaranteed to do so. Even on MRI finalizers can be
+# very unpredictable, due to conservative stack scanning and references
+# left in unused memory.
+
 describe "ObjectSpace.define_finalizer" do
   it "raises an ArgumentError if the action does not respond to call" do
     -> {
-      ObjectSpace.define_finalizer("", mock("ObjectSpace.define_finalizer no #call"))
+      ObjectSpace.define_finalizer(Object.new, mock("ObjectSpace.define_finalizer no #call"))
     }.should raise_error(ArgumentError)
   end
 
   it "accepts an object and a proc" do
-    handler = -> obj { obj }
-    ObjectSpace.define_finalizer("garbage", handler).should == [0, handler]
+    handler = -> id { id }
+    ObjectSpace.define_finalizer(Object.new, handler).should == [0, handler]
+  end
+
+  it "accepts an object and a bound method" do
+    handler = mock("callable")
+    def handler.finalize(id) end
+    finalize = handler.method(:finalize)
+    ObjectSpace.define_finalizer(Object.new, finalize).should == [0, finalize]
   end
 
   it "accepts an object and a callable" do
     handler = mock("callable")
-    def handler.call(obj) end
-    ObjectSpace.define_finalizer("garbage", handler).should == [0, handler]
+    def handler.call(id) end
+    ObjectSpace.define_finalizer(Object.new, handler).should == [0, handler]
+  end
+
+  it "accepts an object and a block" do
+    handler = -> id { id }
+    ObjectSpace.define_finalizer(Object.new, &handler).should == [0, handler]
   end
 
   it "raises ArgumentError trying to define a finalizer on a non-reference" do
@@ -31,7 +49,7 @@ describe "ObjectSpace.define_finalizer" do
   it "calls finalizer on process termination" do
     code = <<-RUBY
       def scoped
-        Proc.new { puts "finalized" }
+        Proc.new { puts "finalizer run" }
       end
       handler = scoped
       obj = "Test"
@@ -39,18 +57,104 @@ describe "ObjectSpace.define_finalizer" do
       exit 0
     RUBY
 
-    ruby_exe(code).should == "finalized\n"
+    ruby_exe(code, :args => "2>&1").should include("finalizer run\n")
   end
 
-  it "calls finalizer at exit even if it is self-referencing" do
+  ruby_version_is "2.8" do
+    it "warns if the finalizer has the object as the receiver" do
+      code = <<-RUBY
+        class CapturesSelf
+          def initialize
+            ObjectSpace.define_finalizer(self, proc {
+              puts "finalizer run"
+            })
+          end
+        end
+        CapturesSelf.new
+        exit 0
+      RUBY
+
+      ruby_exe(code, :args => "2>&1").should include("warning: finalizer references object to be finalized\n")
+    end
+
+    it "warns if the finalizer is a method bound to the receiver" do
+      code = <<-RUBY
+        class CapturesSelf
+          def initialize
+            ObjectSpace.define_finalizer(self, method(:finalize))
+          end
+          def finalize(id)
+            puts "finalizer run"
+          end
+        end
+        CapturesSelf.new
+        exit 0
+      RUBY
+
+      ruby_exe(code, :args => "2>&1").should include("warning: finalizer references object to be finalized\n")
+    end
+
+    it "warns if the finalizer was a block in the reciever" do
+      code = <<-RUBY
+        class CapturesSelf
+          def initialize
+            ObjectSpace.define_finalizer(self) do
+              puts "finalizer run"
+            end
+          end
+        end
+        CapturesSelf.new
+        exit 0
+      RUBY
+
+      ruby_exe(code, :args => "2>&1").should include("warning: finalizer references object to be finalized\n")
+    end
+  end
+
+  it "calls a finalizer at exit even if it is self-referencing" do
     code = <<-RUBY
       obj = "Test"
-      handler = Proc.new { puts "finalized" }
+      handler = Proc.new { puts "finalizer run" }
       ObjectSpace.define_finalizer(obj, handler)
       exit 0
     RUBY
 
-    ruby_exe(code).should == "finalized\n"
+    ruby_exe(code).should include("finalizer run\n")
+  end
+
+  it "calls a finalizer at exit even if it is indirectly self-referencing" do
+    code = <<-RUBY
+      class CapturesSelf
+        def initialize
+          ObjectSpace.define_finalizer(self, finalizer(self))
+        end
+        def finalizer(zelf)
+          proc do
+            puts "finalizer run"
+          end
+        end
+      end
+      CapturesSelf.new
+      exit 0
+    RUBY
+
+    ruby_exe(code, :args => "2>&1").should include("finalizer run\n")
+  end
+
+  it "calls a finalizer defined in a finalizer running at exit" do
+    code = <<-RUBY
+      obj = "Test"
+      handler = Proc.new do
+        obj2 = "Test"
+        handler2 = Proc.new { puts "finalizer 2 run" }
+        ObjectSpace.define_finalizer(obj2, handler2)
+        exit 0
+      end
+      ObjectSpace.define_finalizer(obj, handler)
+      exit 0
+    RUBY
+
+    ruby_exe(code, :args => "2>&1").should include("finalizer 2 run\n")
   end
 
   it "allows multiple finalizers with different 'callables' to be defined" do