sucker_punch seems to have problems with Phusion Passenger #6

Closed
gduprey opened this Issue Feb 26, 2013 · 20 comments

Comments

Projects
None yet
6 participants
@gduprey

gduprey commented Feb 26, 2013

I got sucker_punch working in just a few minutes in my development environment. However, when attempting to deploy into a production environment using Phusion Passenger, I'm seeing problems.

One is that while there are no errors displayed, any attempt to queue a worker/task async doesn't do anything. The call completes, but the worker never seems to run.

The other, which may inform this, is that attempts to query the state of a queue after submitting a task yield a fatal (deadlock detected) error

Specifically, in code that looks like this:

SuckerPunch::Queue[:history_queue].async.perform(Admin::User.current_user.id, new_fav)
puts " ** QUEUED History for SuckerPunch, SIZE=[#{SuckerPunch::Queue[:history_queue].size.to_s}], BUSY=[#{SuckerPunch::Queue[:history_queue].busy_size.to_s}]"

Th line with the puts, trying to query the queue causes the above deadlock to occur. And, per the former note, even though the '....async.perform(xxx)' call doesnt pass an error, the actual worker for this never runs (or at least the perform() method isn't called).

Under development (using the standard 'rails server' webrick server), all works without an error.

Is there anything special about setting up sucker_punch to make it work with tools like passenger?

@gduprey

This comment has been minimized.

Show comment
Hide comment
@gduprey

gduprey Feb 28, 2013

That does seem like the problem. Is there a call that can be used to restart/start sucker_punch's threads? If so, then adding code for this should be pretty simple.

gduprey commented Feb 28, 2013

That does seem like the problem. Is there a call that can be used to restart/start sucker_punch's threads? If so, then adding code for this should be pretty simple.

@FooBarWidget

This comment has been minimized.

Show comment
Hide comment
@FooBarWidget

FooBarWidget Feb 28, 2013

Alternatively you can just use the conservative/direct spawning method.

Alternatively you can just use the conservative/direct spawning method.

@gduprey

This comment has been minimized.

Show comment
Hide comment
@gduprey

gduprey Feb 28, 2013

Well, technically, you could, but there is a pretty significant performance hit for starting new phusion/rails worker. Ideally, if at all possible, we're looking to keep the smart spawning performance benefit.

gduprey commented Feb 28, 2013

Well, technically, you could, but there is a pretty significant performance hit for starting new phusion/rails worker. Ideally, if at all possible, we're looking to keep the smart spawning performance benefit.

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Feb 28, 2013

Owner

I'll try to implement something that will work with it soon.

On Feb 28, 2013, at 3:15 PM, gduprey notifications@github.com wrote:

Well, technically, you could, but there is a pretty significant performance
hit for starting new phusion/rails worker. Ideally, if at all possible,
we're looking to keep the smart spawning performance benefit.


Reply to this email directly or view it on
GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-14254889
.

Owner

brandonhilkert commented Feb 28, 2013

I'll try to implement something that will work with it soon.

On Feb 28, 2013, at 3:15 PM, gduprey notifications@github.com wrote:

Well, technically, you could, but there is a pretty significant performance
hit for starting new phusion/rails worker. Ideally, if at all possible,
we're looking to keep the smart spawning performance benefit.


Reply to this email directly or view it on
GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-14254889
.

@brandonhilkert brandonhilkert referenced this issue in celluloid/celluloid Mar 3, 2013

Closed

Supporting pools after forking #173

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Mar 3, 2013

Owner

The above link about creating some code for after fork. This article talks about doing the same with Redis. You could follow it and just define your queues in a module and then launch that in after fork.

ref: http://ryreitsma.blogspot.com/2011/07/redis-and-phusion-passenger-reconnect.html

Owner

brandonhilkert commented Mar 3, 2013

The above link about creating some code for after fork. This article talks about doing the same with Redis. You could follow it and just define your queues in a module and then launch that in after fork.

ref: http://ryreitsma.blogspot.com/2011/07/redis-and-phusion-passenger-reconnect.html

@gduprey

This comment has been minimized.

Show comment
Hide comment
@gduprey

gduprey Mar 5, 2013

Hmmm -- so to be clear:

  1. When not under passenger, create pools at startup.
  2. When under passenger, do not create at startup, but create after a fork?

So there is no "restarting" of pools -- just defering creation in a passenger environment?

gduprey commented Mar 5, 2013

Hmmm -- so to be clear:

  1. When not under passenger, create pools at startup.
  2. When under passenger, do not create at startup, but create after a fork?

So there is no "restarting" of pools -- just defering creation in a passenger environment?

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Mar 5, 2013

Owner

There is no restarting. If the pools are in another process that Passenger forked, you'd need to make queues in that process since there's not talking between processes.

Re: #2, have you tried defining them at startup, and then in the after_fork portion as well?

Owner

brandonhilkert commented Mar 5, 2013

There is no restarting. If the pools are in another process that Passenger forked, you'd need to make queues in that process since there's not talking between processes.

Re: #2, have you tried defining them at startup, and then in the after_fork portion as well?

@gduprey

This comment has been minimized.

Show comment
Hide comment
@gduprey

gduprey Mar 5, 2013

Thanks. I do get they don't communicate and how working works. For #2 -- I have not tested this yet -- still working on it. While I know the threads would go away, if I started SuckerPunch both at startup and after forking, the class instances setup by SuckerPunch queues would still be around after the fork (but no threads) and likely break things. Better to not start the queues twice.

Since I can't see a way to tell sucker_punch to just drop it's idea of all threads and restart them, it seems smarter to test for passenger and when under it, defer creating the queues until after the fork or, in non-passenger cases, just define the queues at startup.

From what I think is going on in passenger, if I use the hook mentioned above, then passenger will start a "template" process loading the entire environment into it. When a new job comes in and a new thread is needed, it forks the "template" process and then runs the after_fork handlers. So the the "template" process would not setup the queues (as that logic is only in the after_fork) but after forking, they would get setup and run for the rest of the lifetime of that spawned process.

if defined?(PhusionPassenger)
PhusionPassenger.on_event(:starting_worker_process) do |forked|
if forked
# Called after "template" process is forked and about to be used
# Setup the queues
SuckerPunch.config do .... end
end
# Presumably, somewhere in here is 'passenger starting template thread' and we do nothing
# with SuckerPunch
end
else

Non-passenger startup, so just setup queues

SuckerPunch.config do .... end
end

Obviously, as you suggest, it makes sense to put all the SuckerPunch inits in a common place and just have the two invocations from the above code as the environment dictates. I'm working on this now and if the above isn't correct or needs refinement, I'll post it.

gduprey commented Mar 5, 2013

Thanks. I do get they don't communicate and how working works. For #2 -- I have not tested this yet -- still working on it. While I know the threads would go away, if I started SuckerPunch both at startup and after forking, the class instances setup by SuckerPunch queues would still be around after the fork (but no threads) and likely break things. Better to not start the queues twice.

Since I can't see a way to tell sucker_punch to just drop it's idea of all threads and restart them, it seems smarter to test for passenger and when under it, defer creating the queues until after the fork or, in non-passenger cases, just define the queues at startup.

From what I think is going on in passenger, if I use the hook mentioned above, then passenger will start a "template" process loading the entire environment into it. When a new job comes in and a new thread is needed, it forks the "template" process and then runs the after_fork handlers. So the the "template" process would not setup the queues (as that logic is only in the after_fork) but after forking, they would get setup and run for the rest of the lifetime of that spawned process.

if defined?(PhusionPassenger)
PhusionPassenger.on_event(:starting_worker_process) do |forked|
if forked
# Called after "template" process is forked and about to be used
# Setup the queues
SuckerPunch.config do .... end
end
# Presumably, somewhere in here is 'passenger starting template thread' and we do nothing
# with SuckerPunch
end
else

Non-passenger startup, so just setup queues

SuckerPunch.config do .... end
end

Obviously, as you suggest, it makes sense to put all the SuckerPunch inits in a common place and just have the two invocations from the above code as the environment dictates. I'm working on this now and if the above isn't correct or needs refinement, I'll post it.

@topfunky

This comment has been minimized.

Show comment
Hide comment
@topfunky

topfunky Mar 18, 2013

@gduprey Did this work for you? I have several apps that could use a queue like this and they all run on Passenger.

@gduprey Did this work for you? I have several apps that could use a queue like this and they all run on Passenger.

@gduprey

This comment has been minimized.

Show comment
Hide comment
@gduprey

gduprey Mar 18, 2013

Yep -- worked great. Instead of starting the queues in the intitializers, I added logic to a base 'worker' class that, on their first use, dynamically created the queue and then did the perform. Since all my logic that uses these workers is based on user action, they are never started until after passenger has forked the process and thus, no threads to re-create.

Base worker class looked something like:

class ApplicationWorker
  include SuckerPunch::Worker

  class <<  self
    attr_accessor :worker_queue_name

    # Start the appropriate worker queue.  In almost all cases, there is no need to directly call
    # this as the call/spawn calls will invoke it the first time it's needed
    def start_worker
      if @worker_queue_name.nil?
        # Get defaults and set locals with scope
        this_class = self
        the_worker_queue_name = this_class.name.tableize.gsub(/\//, '_').gsub(/_workers/, '_queue').to_sym

        # Actually create the queue
        SuckerPunch.config { queue name: the_worker_queue_name, worker: this_class, size: 5 } 

        # Record the queue name for the future
        @worker_queue_name = the_worker_queue_name
      end
    end

    # Invoke the worker synchronosly.  In short, it's an wasy way to make a call that
    # completes synchronously and also be able to later call it async.  Whatever the
    # perform() method returns is also passed back to the caller.
    def call(*params)
      start_worker                     unless @worker_queue_name
      SuckerPunch::Queue[@worker_queue_name].perform(*params)
    end

    # Invoke the worker asynchronously.  The call will return immediately and the
    # request to execute perform() will be queued and executed as soon as it comes
    # up in the queue.
    def spawn(*params)
      start_worker                     unless @worker_queue_name
      SuckerPunch::Queue[@worker_queue_name].async.perform(*params)
    end
  end

  def perform(...) 
   ....
  end
end

Then just create your worker class as a descendent of ApplicationWorker and the use call() or spawn() on it and it'll handle initial queue creation.

This would not work if you need to spawn off workers during initialization as you'd be right back to the initial problem again.

gduprey commented Mar 18, 2013

Yep -- worked great. Instead of starting the queues in the intitializers, I added logic to a base 'worker' class that, on their first use, dynamically created the queue and then did the perform. Since all my logic that uses these workers is based on user action, they are never started until after passenger has forked the process and thus, no threads to re-create.

Base worker class looked something like:

class ApplicationWorker
  include SuckerPunch::Worker

  class <<  self
    attr_accessor :worker_queue_name

    # Start the appropriate worker queue.  In almost all cases, there is no need to directly call
    # this as the call/spawn calls will invoke it the first time it's needed
    def start_worker
      if @worker_queue_name.nil?
        # Get defaults and set locals with scope
        this_class = self
        the_worker_queue_name = this_class.name.tableize.gsub(/\//, '_').gsub(/_workers/, '_queue').to_sym

        # Actually create the queue
        SuckerPunch.config { queue name: the_worker_queue_name, worker: this_class, size: 5 } 

        # Record the queue name for the future
        @worker_queue_name = the_worker_queue_name
      end
    end

    # Invoke the worker synchronosly.  In short, it's an wasy way to make a call that
    # completes synchronously and also be able to later call it async.  Whatever the
    # perform() method returns is also passed back to the caller.
    def call(*params)
      start_worker                     unless @worker_queue_name
      SuckerPunch::Queue[@worker_queue_name].perform(*params)
    end

    # Invoke the worker asynchronously.  The call will return immediately and the
    # request to execute perform() will be queued and executed as soon as it comes
    # up in the queue.
    def spawn(*params)
      start_worker                     unless @worker_queue_name
      SuckerPunch::Queue[@worker_queue_name].async.perform(*params)
    end
  end

  def perform(...) 
   ....
  end
end

Then just create your worker class as a descendent of ApplicationWorker and the use call() or spawn() on it and it'll handle initial queue creation.

This would not work if you need to spawn off workers during initialization as you'd be right back to the initial problem again.

@topfunky

This comment has been minimized.

Show comment
Hide comment
@topfunky

topfunky Mar 18, 2013

Cool...that works. Thanks!

Cool...that works. Thanks!

@joseph

This comment has been minimized.

Show comment
Hide comment
@joseph

joseph Apr 22, 2013

It seems this isn't Passenger-specific. I'm seeing identical behavior in Unicorn. It's resolved using @gduprey's approach given above. Should this be rolled into Worker?

joseph commented Apr 22, 2013

It seems this isn't Passenger-specific. I'm seeing identical behavior in Unicorn. It's resolved using @gduprey's approach given above. Should this be rolled into Worker?

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Apr 22, 2013

Owner

See the readme. There are details about how to set it up for both unicorn
and passenger.

IMO, software isn't a concern of this gem.
On Apr 22, 2013 6:43 PM, "Joseph Pearson" notifications@github.com wrote:

It seems this isn't Passenger-specific. I'm seeing identical behavior in
Unicorn. It's resolved using @gduprey https://github.com/gduprey's
approach given above. Should this be rolled into Worker?


Reply to this email directly or view it on GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-16828691
.

Owner

brandonhilkert commented Apr 22, 2013

See the readme. There are details about how to set it up for both unicorn
and passenger.

IMO, software isn't a concern of this gem.
On Apr 22, 2013 6:43 PM, "Joseph Pearson" notifications@github.com wrote:

It seems this isn't Passenger-specific. I'm seeing identical behavior in
Unicorn. It's resolved using @gduprey https://github.com/gduprey's
approach given above. Should this be rolled into Worker?


Reply to this email directly or view it on GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-16828691
.

@joseph

This comment has been minimized.

Show comment
Hide comment
@joseph

joseph Apr 22, 2013

Oh yeah. Evidently I didn't scroll down all the way. Looks like that'll do it, thanks.

joseph commented Apr 22, 2013

Oh yeah. Evidently I didn't scroll down all the way. Looks like that'll do it, thanks.

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Apr 22, 2013

Owner

Sounds good. Let me know if there's anything that would make it clearer.
On Apr 22, 2013 7:22 PM, "Joseph Pearson" notifications@github.com wrote:

Oh yeah. Evidently I didn't scroll down all the way. Looks like that'll do
it, thanks.


Reply to this email directly or view it on GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-16830224
.

Owner

brandonhilkert commented Apr 22, 2013

Sounds good. Let me know if there's anything that would make it clearer.
On Apr 22, 2013 7:22 PM, "Joseph Pearson" notifications@github.com wrote:

Oh yeah. Evidently I didn't scroll down all the way. Looks like that'll do
it, thanks.


Reply to this email directly or view it on GitHubhttps://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-16830224
.

@richww

This comment has been minimized.

Show comment
Hide comment
@richww

richww Jan 14, 2016

Hi folks,

I'm still running into this issue on Passenger 5.0.21 / Ruby 2.2.2 / Rails 4.2.5 / sucker_punch 1.6.0, even though according to this section of the README the issue should now be resolved:

Previously, Sucker Punch required an initializer and that posed problems for Unicorn and Passenger and other servers that fork. Version 1 was rewritten to not require any special code to be executed after forking occurs. Please remove if you're using version >= 1.0.0

I have an issue where a SuckerPunch Job is not executing asynchronously. On Webrick, it works fine, but on Passenger nothing happens.

I confirmed that my Passenger is using "Smart spawning" and therefore forking, and I also verified that forcing "Direct spawning" in Apache's httpd.conf, the Job works as expected.

So, I had to add this block to an initializer to ensure the Celluloid worker pool was running on the forked Passenger processes:

if defined?(PhusionPassenger)
  PhusionPassenger.on_event(:starting_worker_process) do |forked|
    if forked
      pool_class = [My::Suckerpunch::Job::Class]
      pool_name = "sucker_punch_[my_suckerpunch_job_class]"
      pool = Class.new(Celluloid::Supervision::Container) do
           pool pool_class, as: pool_name, size: num_workers
      end
      pool.run!
    end
  end
end

Regarding why I need to run the pool for each forked process, I think I see where the problem is. See queue.rb here:

def registered?
  Celluloid::Actor.registered.include?(name.to_sym)
end

This is returning true within the forked Passenger thread, so Celluloid is representing the actor as registered for our Job, so it skips the initialize_celluloid_pool. The pool isn't running so no jobs are executed.

So, am I missing something? What was done to fix this issue within sucker_punch 1.0? Or perhaps a recent update to sucker-punch or celluloid has brought this problem back?

Thanks for any help or insight you could provide.

richww commented Jan 14, 2016

Hi folks,

I'm still running into this issue on Passenger 5.0.21 / Ruby 2.2.2 / Rails 4.2.5 / sucker_punch 1.6.0, even though according to this section of the README the issue should now be resolved:

Previously, Sucker Punch required an initializer and that posed problems for Unicorn and Passenger and other servers that fork. Version 1 was rewritten to not require any special code to be executed after forking occurs. Please remove if you're using version >= 1.0.0

I have an issue where a SuckerPunch Job is not executing asynchronously. On Webrick, it works fine, but on Passenger nothing happens.

I confirmed that my Passenger is using "Smart spawning" and therefore forking, and I also verified that forcing "Direct spawning" in Apache's httpd.conf, the Job works as expected.

So, I had to add this block to an initializer to ensure the Celluloid worker pool was running on the forked Passenger processes:

if defined?(PhusionPassenger)
  PhusionPassenger.on_event(:starting_worker_process) do |forked|
    if forked
      pool_class = [My::Suckerpunch::Job::Class]
      pool_name = "sucker_punch_[my_suckerpunch_job_class]"
      pool = Class.new(Celluloid::Supervision::Container) do
           pool pool_class, as: pool_name, size: num_workers
      end
      pool.run!
    end
  end
end

Regarding why I need to run the pool for each forked process, I think I see where the problem is. See queue.rb here:

def registered?
  Celluloid::Actor.registered.include?(name.to_sym)
end

This is returning true within the forked Passenger thread, so Celluloid is representing the actor as registered for our Job, so it skips the initialize_celluloid_pool. The pool isn't running so no jobs are executed.

So, am I missing something? What was done to fix this issue within sucker_punch 1.0? Or perhaps a recent update to sucker-punch or celluloid has brought this problem back?

Thanks for any help or insight you could provide.

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Jan 15, 2016

Owner

@richww Can you give the latest 2.0.0 beta a shot? I'm not inclined to go back and dig in to that version at this point.

Owner

brandonhilkert commented Jan 15, 2016

@richww Can you give the latest 2.0.0 beta a shot? I'm not inclined to go back and dig in to that version at this point.

@richww

This comment has been minimized.

Show comment
Hide comment
@richww

richww Jan 26, 2016

@brandonhilkert Yup, 2.0.0 seems to fix the Passenger smart spawning issue for me, thanks!

richww commented Jan 26, 2016

@brandonhilkert Yup, 2.0.0 seems to fix the Passenger smart spawning issue for me, thanks!

@brandonhilkert

This comment has been minimized.

Show comment
Hide comment
@brandonhilkert

brandonhilkert Jan 26, 2016

Owner

Great news! Thanks for the feedback!

On Tuesday, January 26, 2016, richww notifications@github.com wrote:

@brandonhilkert https://github.com/brandonhilkert Yup, 2.0.0 seems to
fix the Passenger smart spawning issue for me, thanks!


Reply to this email directly or view it on GitHub
#6 (comment)
.


Build a Ruby Gem is available!
http://brandonhilkert.com/books/build-a-ruby-gem/?utm_source=gmail-sig&utm_medium=email&utm_campaign=gmail

http://brandonhilkert.com

Owner

brandonhilkert commented Jan 26, 2016

Great news! Thanks for the feedback!

On Tuesday, January 26, 2016, richww notifications@github.com wrote:

@brandonhilkert https://github.com/brandonhilkert Yup, 2.0.0 seems to
fix the Passenger smart spawning issue for me, thanks!


Reply to this email directly or view it on GitHub
#6 (comment)
.


Build a Ruby Gem is available!
http://brandonhilkert.com/books/build-a-ruby-gem/?utm_source=gmail-sig&utm_medium=email&utm_campaign=gmail

http://brandonhilkert.com

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