Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

deprecate Proc#bind that can cause symbol memory leak

  • Loading branch information...
commit 9c857db75788c21f6184279c130e79f21c750f9f 1 parent 6424922
@lest lest authored
View
3  activesupport/lib/active_support/core_ext/proc.rb
@@ -1,7 +1,10 @@
require "active_support/core_ext/kernel/singleton_class"
+require "active_support/deprecation"
class Proc #:nodoc:
def bind(object)
+ ActiveSupport::Deprecation.warn 'Proc#bind is deprecated and will be removed in future versions', caller
+
block, time = self, Time.now
object.class_eval do
method_name = "__bind_#{time.to_i}_#{time.usec}"
View
2  activesupport/lib/active_support/rescuable.rb
@@ -108,7 +108,7 @@ def handler_for_rescue(exception)
when Symbol
method(rescuer)
when Proc
- rescuer.bind(self)
+ Proc.new { |*args| instance_exec(*args, &rescuer) }
end
end
end
View
12 activesupport/test/core_ext/proc_test.rb
@@ -3,10 +3,12 @@
class ProcTests < ActiveSupport::TestCase
def test_bind_returns_method_with_changed_self
- block = Proc.new { self }
- assert_equal self, block.call
- bound_block = block.bind("hello")
- assert_not_equal block, bound_block
- assert_equal "hello", bound_block.call
+ assert_deprecated do
+ block = Proc.new { self }
+ assert_equal self, block.call
+ bound_block = block.bind("hello")
+ assert_not_equal block, bound_block
+ assert_equal "hello", bound_block.call
+ end
end
end

5 comments on commit 9c857db

@davidmiani

I recently asked a question on stackoverflow about rebinding self on a proc that takes a block, and the solution given was to use the Proc#bind method that you just depreciated. Since instance_exec doesn't fully replace it, is it a good idea to depreciate it? I thought a better solution would be to check if the Proc takes a block. If it doesn't, then call instance_exec, otherwise use the existing method. That would stop the majority of leaks (in cases where the Proc doesn't take a block), but not reduce functionality. I was thinking something like this:

def bind(object)
    if parameters.any? && parameters.last.first == :block
        #...existing bind code
    else
        Proc.new {|*args| object.instance_exec(*args, &self)}
    end
end
@drogus
Collaborator

@nanothief the problem with bind is that it can create a lot of symbols, I'm not sure how does your code can prevent that. If you need Proc.bind and it works for you, please extract it to a gem or move to you application.

@davidmiani

@drogus You are right, my previous code only stopped the memory leak when the Proc didn't use a block, not all the time.

I thought about it a bit more, and I realized the problem comes from these two lines:

method_name = "__bind_#{time.to_i}_#{time.usec}"
define_method(method_name, &block)

For every invocation of bind, this is creating a unique symbol to create the method with. This does leak very quickly, the code:

1000000.times do
    p = Proc.new { |a,b| a * b }
    p = p.bind("hello")
end

Takes 31 seconds to run for me, and at its peak consumes 210MB of memory.

To fix this, I wrote a version that only uses one symbol. It is a fairly unique symbol, meaning it would be pretty unlikely for it to be added to another class. Even if it is, the program will capture the old method and rebind it after the method is finished. This version takes 13 seconds to run the above code, and the memory used didn't vary much from between 25-35MB.

 class Proc
    def bind(object)
        block = self
        object.class_eval do
            method_name = :__proc_rebound_method__
            method_already_exists = 
                object.respond_to?(method_name) &&
                instance_method(method_name).owner == self

            old_method = instance_method(method_name) if method_already_exists

            define_method(method_name, &block)
            method = instance_method(method_name)
            remove_method(method_name)

            define_method(method_name, old_method) if method_already_exists
            method
        end.bind(object)
    end
end
@drogus
Collaborator

What will happen if you want to bind 2 proc objects to one class?

@davidmiani

It will be fine, as the new method is removed before the bind method finishes.

However thinking about that, in a multithreaded situation, you could hit problems. I couldn't manage to produce a situation where it would produce incorrect output, however the following could be safer:

class Proc
    def bind(object)
        block = self
        object.class_eval do
            method_name = :__proc_rebound_method__
            method = nil
            Thread.exclusive do
                method_already_exists = 
                    object.respond_to?(method_name) &&
                    instance_method(method_name).owner == self

                old_method = instance_method(method_name) if method_already_exists

                define_method(method_name, &block)
                method = instance_method(method_name)
                remove_method(method_name)

                define_method(method_name, old_method) if method_already_exists
            end
            method
        end.bind(object)
    end
end
Please sign in to comment.
Something went wrong with that request. Please try again.