Skip to content

Fix memory leak: do not add trait duplications to @defined_traits #588

Closed
wants to merge 4 commits into from

3 participants

@greyblake

Each call of FactoryGir.build() with trait leads to adding an instance of FactoryGirl::Trait to @defined_traits array. After ~1 hour of running tests in huge application it results into serious memory leaks (in our case it took over 14Gb of RAM, when usually 1Gb is enough) and specs never end, seems almost all the time are used by GC. With debugger and ObjectSpace I detected about 10 arrays with size over 32 000 000 elements, but applying array.uniq.size returns only 15. So they store duplicated references to same objects.

How to Reproduce:

I've created a demo application for it: https://github.com/greyblake/factory_memory_leak

In short:

  • Create a model rails g user login:string name:string
  • Create a factory:
  factory :user do
    login "hack68"

    trait :with_name do
      name "Mr. Smith"
    end
  end
  • Create a test to demonstrate the leak:
require 'test_helper'

class UserTest < ActiveSupport::TestCase
  test "the memory leak" do
    10.times do |index|
      puts "\nIteration: #{index}"

      ObjectSpace.each_object(Array) do |array| if array.last.is_a?(FactoryGirl::Trait) && array.size > 1
          puts "Traits: #{array.size}"
          puts "Unique traits: #{array.uniq.size}"
        end
      end

      FactoryGirl.build(:user, :with_name)
    end
  end
end
  • Run the test:
~/dev/work/tmx/factory_memory_leak[master]$ rake test
Run options: 

# Running tests:

[1/1] UserTest#test_the_memory_leak
Iteration: 0

Iteration: 1

Iteration: 2
Traits: 2
Unique traits: 1

Iteration: 3
Traits: 3
Unique traits: 1

Iteration: 4
Traits: 4
Unique traits: 1

Iteration: 5
Traits: 5
Unique traits: 1

Iteration: 6
Traits: 6
Unique traits: 1

So, every call of build increments the trait array by 1 (at least in this simple app). Using ruby-mass I found out that the array is defined_tratis proprety of FactoryGirl::Definition

Environment

  • Ruby 1.9.3 / 2.0.0 / 2.1.0
  • Rails 3.2.15
  • FactoryGirl: 4.3.0
  • OS: Linux Debian Squeeze
@jferris jferris and 1 other commented on an outdated diff Dec 11, 2013
lib/factory_girl/definition.rb
@@ -77,7 +77,7 @@ def skip_create
end
def define_trait(trait)
- @defined_traits << trait
+ @defined_traits << trait unless @defined_traits.include?(trait)
@jferris
thoughtbot, inc. member
jferris added a note Dec 11, 2013

Is order important for these? How about using a Set?

@greyblake
greyblake added a note Dec 11, 2013

Looks like not. Good idea!
I addressed it in b3e7f7d.
But it made me to add some additional changes to make all specs pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@joshuaclayton
thoughtbot, inc. member

Does Set support any?? If so, you could return:

instance.defined_traits.any? do |trait|
  trait.name == trait_name && trait.send(:block) == @block
end
@greyblake

@joshuaclayton Thanks! Addressed in 938b17e

@joshuaclayton
thoughtbot, inc. member

@greyblake Thanks, looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.