sucker_punch seems to have problems with Phusion Passenger

[gduprey]

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?

[FooBarWidget]

http://www.modrails.com/documentation/Users%20guide%20Apache.html#_smart_spawning_gotcha_2_the_need_to_revive_threads

[gduprey]

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]

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

[gduprey]

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]

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]

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]

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]

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]

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]

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

[gduprey]

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]

Cool...that works. Thanks!

[joseph]

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]

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]

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

[brandonhilkert]

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]

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]

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

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

[brandonhilkert]

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 https://github.com/brandonhilkert/sucker_punch/issues/6#issuecomment-175223450 .

---

_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