deprecate Proc#bind that can cause symbol memory leak #5552

Merged
merged 1 commit into from Mar 22, 2012

Projects

None yet

4 participants

@lest

https://groups.google.com/forum/#!msg/rubyonrails-core/ucD1rQ2KbpU/mys8W5xHmsUJ

if i run my application for long enough time i start getting errors like this:

RuntimeError: symbol table overflow (symbol __bind_1328993330_18...)

apparently it's because symbols are never GCed and rails generates one every time this proc extension is used because method names are implicitly converted to symbols
@tenderlove tenderlove merged commit fad83d8 into rails:master Mar 22, 2012
@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
Ruby on Rails member

@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.

@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
Ruby on Rails member

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

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
@rtomayko rtomayko referenced this pull request in SamSaffron/MiniProfiler Sep 14, 2012
Merged

Avoid deprecated Proc#bind when instance_exec available #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment