With Unicorn, production mode, and unicorn's preload_app, work isn't being completed #47

Closed
scottkf opened this Issue Jan 4, 2012 · 15 comments

Comments

Projects
None yet
7 participants
@scottkf

scottkf commented Jan 4, 2012

I saw the older issue, but this again crops up where work isn't being completed if preload_app is true.

It does work when put into after_fork, however.

@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Mar 7, 2012

I can verify the problem I had to add my queue definitions to the after_fork block

after_fork do |server, worker|
  # Replace with MongoDB or whatever
  if defined?(ActiveRecord::Base)
    ActiveRecord::Base.establish_connection
    Rails.logger.info('Connected to ActiveRecord')
  end

  # If you are using Redis but not Resque, change this
  if defined?(Resque)
    Resque.redis = ENV['REDIS_URI']
    Rails.logger.info('Connected to Redis')
  end


  # Girl Friday Config
  ::COMMENT_QUEUE = GirlFriday::WorkQueue.new(:comment_queue, :size => 3) do |info|
    CommentMailer.mail_subscribers! info[:id]
  end

  ::DEVISE_QUEUE = GirlFriday::WorkQueue.new(:devise_queue, :size => 3) do |info|
    DeviseBackgroundMailer.complete_action(info).deliver
  end

  ::INVITE_QUEUE = GirlFriday::WorkQueue.new(:invite_queue, :size => 3) do |info|
    InviteMailer.invite_email(info[:id]).deliver
  end
end

I can verify the problem I had to add my queue definitions to the after_fork block

after_fork do |server, worker|
  # Replace with MongoDB or whatever
  if defined?(ActiveRecord::Base)
    ActiveRecord::Base.establish_connection
    Rails.logger.info('Connected to ActiveRecord')
  end

  # If you are using Redis but not Resque, change this
  if defined?(Resque)
    Resque.redis = ENV['REDIS_URI']
    Rails.logger.info('Connected to Redis')
  end


  # Girl Friday Config
  ::COMMENT_QUEUE = GirlFriday::WorkQueue.new(:comment_queue, :size => 3) do |info|
    CommentMailer.mail_subscribers! info[:id]
  end

  ::DEVISE_QUEUE = GirlFriday::WorkQueue.new(:devise_queue, :size => 3) do |info|
    DeviseBackgroundMailer.complete_action(info).deliver
  end

  ::INVITE_QUEUE = GirlFriday::WorkQueue.new(:invite_queue, :size => 3) do |info|
    InviteMailer.invite_email(info[:id]).deliver
  end
end
@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Mar 8, 2012

I've got a better way to do this now. I use the Rails singleton model to lazily instantiate the queues. Now I don't need to pollute my unicorn config file and my RSpec tests running without unicorn still have the queues instantiated. The above code translates to.

----------------------
app/models/comment_queue.rb
----------------------
  1 class CommentQueue < GirlFriday::WorkQueue
  2   include Singleton
  3  
  4   def initialize
  5     super(:comment_queue, :size => 3) do |info|
  6       CommentMailer.mail_subscribers! info[:id]
  7     end
  8   end
  9  
 10   def self.push *args
 11     instance.push *args
 12   end
 13  
 14 end

repeated for each queue. This pattern could be folded into girl friday's internals with a bit of metaprogramming

I've got a better way to do this now. I use the Rails singleton model to lazily instantiate the queues. Now I don't need to pollute my unicorn config file and my RSpec tests running without unicorn still have the queues instantiated. The above code translates to.

----------------------
app/models/comment_queue.rb
----------------------
  1 class CommentQueue < GirlFriday::WorkQueue
  2   include Singleton
  3  
  4   def initialize
  5     super(:comment_queue, :size => 3) do |info|
  6       CommentMailer.mail_subscribers! info[:id]
  7     end
  8   end
  9  
 10   def self.push *args
 11     instance.push *args
 12   end
 13  
 14 end

repeated for each queue. This pattern could be folded into girl friday's internals with a bit of metaprogramming

@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Mar 8, 2012

Even nicer

class LazyWorkQueue < GirlFriday::WorkQueue

  include Singleton

  def self.define *options, &block
    Class.new LazyWorkQueue do
      define_method :initialize do
        super *options do |info|
          block.call
        end
      end
    end
  end

  def self.push *args
    instance.push *args
  end

  def self.status
    instance.status
  end

end

and I can do

InviteQueue = LazyWorkQueue.define :invite_queue, :size => 3 do |info|
  InviteMailer.invite_email(info[:id]).deliver
end

which is pretty much the same as the original code except that the queue is lazily instantiated which
works with unicorn and rspec.

Even nicer

class LazyWorkQueue < GirlFriday::WorkQueue

  include Singleton

  def self.define *options, &block
    Class.new LazyWorkQueue do
      define_method :initialize do
        super *options do |info|
          block.call
        end
      end
    end
  end

  def self.push *args
    instance.push *args
  end

  def self.status
    instance.status
  end

end

and I can do

InviteQueue = LazyWorkQueue.define :invite_queue, :size => 3 do |info|
  InviteMailer.invite_email(info[:id]).deliver
end

which is pretty much the same as the original code except that the queue is lazily instantiated which
works with unicorn and rspec.

@mperham

This comment has been minimized.

Show comment
Hide comment
Owner

mperham commented Mar 8, 2012

@bradphelan Nice!

@mperham mperham closed this Mar 8, 2012

@tcassanego

This comment has been minimized.

Show comment
Hide comment
@tcassanego

tcassanego Mar 21, 2012

@bradphelan Thanks for those solutions! I ended up using the first as it seemed quite nice to have an app model representing the queue(s).

@mperham Would it make sense to drop a small blurb in the wiki docs pointing people here if they're using unicorn? I ended up adding the preload_app to my unicorn config since that seems to be the recommended approach with unicorn on heroku. It was tough to eventually find this since I didn't realize preload_app was the problem until much later.

Thanks!

@bradphelan Thanks for those solutions! I ended up using the first as it seemed quite nice to have an app model representing the queue(s).

@mperham Would it make sense to drop a small blurb in the wiki docs pointing people here if they're using unicorn? I ended up adding the preload_app to my unicorn config since that seems to be the recommended approach with unicorn on heroku. It was tough to eventually find this since I didn't realize preload_app was the problem until much later.

Thanks!

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Mar 21, 2012

Owner

@tcassanego Sounds great. Why don't you add it to the wiki yourself? It's publicly editable.

Owner

mperham commented Mar 21, 2012

@tcassanego Sounds great. Why don't you add it to the wiki yourself? It's publicly editable.

@tcassanego

This comment has been minimized.

Show comment
Hide comment
@tcassanego

tcassanego Mar 23, 2012

@mperham Ah! Didn't realize. Will do!

@mperham Ah! Didn't realize. Will do!

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 23, 2012

I'd be careful with using a global singleton to lazily create queues and thread-safety. If two threads try to use the queues at 'just' the right time, they may end up both trying to 'initialize' the queues, with potentially odd results, depending on exactly how your code works (the different versions brad supplies may even differ in whether they are subject to this flaw, I can't tell for sure from a skim).

May not end up being a problem in GIL rubies.

But may be safest to just use the after_fork method (or passenger equivalent) and not have to worry about it.

I'd be careful with using a global singleton to lazily create queues and thread-safety. If two threads try to use the queues at 'just' the right time, they may end up both trying to 'initialize' the queues, with potentially odd results, depending on exactly how your code works (the different versions brad supplies may even differ in whether they are subject to this flaw, I can't tell for sure from a skim).

May not end up being a problem in GIL rubies.

But may be safest to just use the after_fork method (or passenger equivalent) and not have to worry about it.

@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Mar 23, 2012

@jrochkind You bring up a legit concern but Ruby 1.9.2's Singleton mixin is threadsafe :)

http://ruby-doc.org/stdlib-1.9.2/libdoc/singleton/rdoc/Singleton.html

See the source code for the init method.

@jrochkind You bring up a legit concern but Ruby 1.9.2's Singleton mixin is threadsafe :)

http://ruby-doc.org/stdlib-1.9.2/libdoc/singleton/rdoc/Singleton.html

See the source code for the init method.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 23, 2012

Cool, the important thing there is that you're creating the queue in
the singleton's constructor, that's what makes singleton's
threadsafe-ness apply to queue creation here, if you were creating it
lazily in some other method you'd have to take care of synchronization
yourself.

It also means that there's a little bit of added overhead for the
mutex every time any thread asks for the singleton object. Probably
not a big deal.

I've just learned in multi-threaded programming, the simpler you keep
things and the fewer mutexes the better -- I think I'd still be using
the on_fork methods, but the singleton one should work, I agree!

Cool, the important thing there is that you're creating the queue in
the singleton's constructor, that's what makes singleton's
threadsafe-ness apply to queue creation here, if you were creating it
lazily in some other method you'd have to take care of synchronization
yourself.

It also means that there's a little bit of added overhead for the
mutex every time any thread asks for the singleton object. Probably
not a big deal.

I've just learned in multi-threaded programming, the simpler you keep
things and the fewer mutexes the better -- I think I'd still be using
the on_fork methods, but the singleton one should work, I agree!

@bradphelan

This comment has been minimized.

Show comment
Hide comment
@bradphelan

bradphelan Mar 23, 2012

@jrochkind You should look at the singleton code closely. The only time mutexes are used are if the instance needs creating. Otherwise the instance is just returned with no mutex overhead.

 def klass.instance
    return @singleton__instance__ if @singleton__instance__
    @singleton__mutex__.synchronize {
      return @singleton__instance__ if @singleton__instance__
      @singleton__instance__ = new()
    }
    @singleton__instance__
  end

The only reason for on_fork is to clean up IO handles that have been duplicated across the fork call. There is no reason for girl_friday to create those IO handles before they are actually needed. on_fork callback in unicorn is a bit of a hack to fix things that probably should be handled in a cleaner hidden way from the library user.

@jrochkind You should look at the singleton code closely. The only time mutexes are used are if the instance needs creating. Otherwise the instance is just returned with no mutex overhead.

 def klass.instance
    return @singleton__instance__ if @singleton__instance__
    @singleton__mutex__.synchronize {
      return @singleton__instance__ if @singleton__instance__
      @singleton__instance__ = new()
    }
    @singleton__instance__
  end

The only reason for on_fork is to clean up IO handles that have been duplicated across the fork call. There is no reason for girl_friday to create those IO handles before they are actually needed. on_fork callback in unicorn is a bit of a hack to fix things that probably should be handled in a cleaner hidden way from the library user.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Mar 23, 2012

Aha, thanks, awesome. As far as cleaning up IO handles -- girl_friday
itself may create/use such IO handles that need to be cleaned up? So
for use with Passenger, someone should take a look at what's being
done for unicorn and try to do similar in passenger?

Aha, thanks, awesome. As far as cleaning up IO handles -- girl_friday
itself may create/use such IO handles that need to be cleaned up? So
for use with Passenger, someone should take a look at what's being
done for unicorn and try to do similar in passenger?

@gluemonkey

This comment has been minimized.

Show comment
Hide comment
@gluemonkey

gluemonkey Dec 4, 2012

@bradphelan Dude, brilliant and a real lifesaver! Thanks!

I did have to make a slight modification to get that info object across in my implementation.

class LazyWorkQueue < GirlFriday::WorkQueue

  include Singleton

  def self.define *options, &block
    Class.new LazyWorkQueue do
      define_method :initialize do
        super *options do |info|
          block.call(info)
        end
      end
    end
  end

  def self.push *args
    instance.push *args
  end

  def self.status
    instance.status
  end

end

@bradphelan Dude, brilliant and a real lifesaver! Thanks!

I did have to make a slight modification to get that info object across in my implementation.

class LazyWorkQueue < GirlFriday::WorkQueue

  include Singleton

  def self.define *options, &block
    Class.new LazyWorkQueue do
      define_method :initialize do
        super *options do |info|
          block.call(info)
        end
      end
    end
  end

  def self.push *args
    instance.push *args
  end

  def self.status
    instance.status
  end

end
@fallanic

This comment has been minimized.

Show comment
Hide comment
@fallanic

fallanic Jan 30, 2013

I don't know if there's a more elegant solution but @gluemonkey 's patch worked for me (and huge thanks to @bradphelan)

@mperham shall we copy this piece of code in the wiki?

I don't know if there's a more elegant solution but @gluemonkey 's patch worked for me (and huge thanks to @bradphelan)

@mperham shall we copy this piece of code in the wiki?

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Jan 30, 2013

Owner

That lazy implementation looks nice and elegant. I like the use of Singleton to delay the instance creation. Maybe we should pull it into girl_friday as the new WorkQueue?

Owner

mperham commented Jan 30, 2013

That lazy implementation looks nice and elegant. I like the use of Singleton to delay the instance creation. Maybe we should pull it into girl_friday as the new WorkQueue?

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