berkshelf / ridley

A reliable Chef API client with a clean syntax
Other
231 stars 85 forks source link

Ridley doesn't clean up its celluloid actors if it's destroyed/GCed. #283

Open Igneous opened 9 years ago

Igneous commented 9 years ago

If you instantiate ridley in a block which should be GCed after run, like:

1.times do
  ridley = Ridley.from_chef_config
end

When the Ridley class gets destroyed, there's no finalizer to clean up the celluloid threads that are spawned because of it. That example is trivial, but in an instance where it's being instantiated from inside of a route in a web framework (like a sinatra get block, or before, etc) this can very quickly baloon your ruby VM and cause an OOM.

I've worked around it by changing my code to essentially use a global ridley object that gets passed around through dependency injection (which is probably what I should've been doing anyway), but It'd be nice if the ridley class had finalizer to clean up those celluloid actors.

Igneous commented 9 years ago

So.. I've dug through your Ridley::Client and it seems like there is in fact a finalizer that calls finalize_callback. I'm not sure why this wouldn't fire in the above scenario, but I don't think it is :frowning:

sethvargo commented 9 years ago

@Igneous this could have been a bug in Celluloid. We just upgraded to the latest released version of Celluloid - could you try and see if that fixes your issue?

Igneous commented 9 years ago

Same thing, give it a whirl yourself:

#!/usr/bin/env ruby
require 'ridley'
require 'json'

def instantiate_ridley_and_return_nil
  Ridley.from_chef_config
  return nil
end

def print_stats(state)
  threads = Thread.list
  objects = ObjectSpace.count_objects
  puts "#{state} objcount: #{JSON.pretty_generate(objects)}"
  puts "#{state} threads (num = #{threads.count}): #{JSON.pretty_generate(threads)}"
end

print_stats("BEFORE LOOP")

ARGV.first.to_i.times do |count|
  puts '----------------------------------------------------'
  print_stats("BEFORE RUN")
  puts "loop ##{count}: Instantiating ridley"
  instantiate_ridley_and_return_nil
  puts "Ridley (loop #{count}) instantiation finished -- should be destroyed"
  print_stats("AFTER RUN")
  puts '----------------------------------------------------'
  puts
end

print_stats("AFTER LOOP")

Run it with ./test.rb 5 to spawn five instances of ridley, which, (I think) should be finalized immediately after that method call completes.. Hopefully, you'll see that after each loop, the object count and the thread count continues to increase.

If you want, you can add a sleep at the end of the script to see if those threads eventually terminate, but in my experience those celluloid actors stick around as long as the vm that spawned them stays running.