brandonhilkert / sucker_punch

Sucker Punch is a Ruby asynchronous processing library using concurrent-ruby, heavily influenced by Sidekiq and girl_friday.
MIT License
2.65k stars 114 forks source link

perform_async doesn't work with Sinatra in classic mode #206

Closed dare05 closed 6 years ago

dare05 commented 6 years ago

But it works all well when Sinatra is in modular mode. Have anyone else experienced the same?

Here's a sample code:

require 'sinatra'
require 'sucker_punch'

class ValidateJob
  include SuckerPunch::Job

  def perform
    File.write('test.txt', 'test')
  end
end

get '/' do
  ValidateJob.perform_async
end
brandonhilkert commented 6 years ago

Happy to investigate if you provide any one of the following: sample app, any error messages, expectations, helpful debuggging.

On Mon, Nov 6, 2017 at 8:27 AM Darko notifications@github.com wrote:

But it works all well when Sinatra is in modular mode. Have anyone else experienced the same?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/brandonhilkert/sucker_punch/issues/206, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtbFDO2owS2XexPyLHPGV5Ncpc-Iprtks5szwkrgaJpZM4QTPG7 .

--


http://brandonhilkert.com

dare05 commented 6 years ago

@brandonhilkert just provided a sample app.

brandonhilkert commented 6 years ago

It's works for me if I use puma as the application server. How did you invoke the server?

dare05 commented 6 years ago

Uh doesn't work both on Puma/Thin. I think it has to do with the fact that the running OS is Windows 7.

brandonhilkert commented 6 years ago

@dare05 It's possible. I don't have a Windows machine, so I'm not sure I'll be much help.

dare05 commented 6 years ago

@brandonhilkert I'd love to be able to diagnose it but it's a very silent failure.

brandonhilkert commented 6 years ago

@dare05 Sucker Punch uses https://github.com/ruby-concurrency/concurrent-ruby under the covers. You could install that library in to the app and then manually create a thread poll and see if you can get some output from it. It might be a bug with that lib.

richardkmichael commented 6 years ago

@brandonhilkert

@dare05

UPDATE: There is no bug. :-) Sinatra uses at_exit() to run itself:

https://github.com/sinatra/sinatra/blob/master/lib/sinatra/main.rb#L26

at_exit handlers are run in reverse order.

Hence, SuckerPunch must be required before Sinatra, to avoid SuckerPunch shutting itself down when its at_exit() handler is called as Sinatra is "exiting" .. so that it can start up! (Details from my investigation left below.)

# app.rb

require 'sucker_punch'
require 'sinatra'
...

Investigation details for posterity

SuckerPunch's at_exit() handler is called immediately on Sinatra startup.

$  # or `ruby app.rb` with "require 'bundler' ; Bundler.setup" in app.rb
$ bundle exec ruby app.rb
Called: self.shutdown_all(), puts added to queue.rb
[2017-12-12 14:51:56] INFO  WEBrick 1.3.1
[2017-12-12 14:51:56] INFO  ruby 2.4.2 (2017-09-14) [x86_64-darwin16]
== Sinatra (v2.0.0) has taken the stage on 4567 for development with backup from WEBrick
[2017-12-12 14:51:56] INFO  WEBrick::HTTPServer#start: pid=13924 port=4567

https://github.com/brandonhilkert/sucker_punch/blob/9f9a04d494c483b780a39d27049a52aadc1dc4e6/lib/sucker_punch.rb#L53-L55

The handler calls SuckerPunch::Queue.shutdown_all() which calls SuckerPunch::RUNNING.make_false:

`https://github.com/brandonhilkert/sucker_punch/blob/9f9a04d494c483b780a39d27049a52aadc1dc4e6/lib/sucker_punch/queue.rb#L71-L74

I see #make_false comes from concurrent-ruby (looks like it's a helper for thread-safe state/variables?). So either it has a bug and it should not be succeeding (that is, make_false should return false here); or, SuckerPunch should not be calling make_false (or something previous in the code path.. maybe the queue shutdown shouldn't be happening?). In any case, at this point, SuckerPunch believes it is not running, i.e., SuckerPunch::RUNNING.true? is false.

Later, perform_async() returns immediately if SuckerPunch is not running:

https://github.com/brandonhilkert/sucker_punch/blob/9f9a04d494c483b780a39d27049a52aadc1dc4e6/lib/sucker_punch/job.rb#L33-L34

Commenting the short-circuiting return here will allow perform_async jobs to run. But, obviously this is not the "fix"! :-)

Here are the test app and gem details:

app.rb

# require 'bundler'
# Bundler.setup

require 'sinatra'
require 'sucker_punch'

SuckerPunch.logger = Logger.new('sucker_punch.log')

class ValidateJob
  include SuckerPunch::Job

  def perform
    SuckerPunch.logger.info("perform start")
    sleep 5
    SuckerPunch.logger.info("perform end")
  end
end

# set :server, "thin" # "puma"

get '/' do
  ValidateJob.perform_async # SuckerPunch::RUNNING.true?  ==> false, oops
  # ValidateJob.new.perform # SuckerPunch::RUNNING.true?  ==> never called
end

Sinatra classic startup: (WEBrick, but thin and puma have the same behaviour)

$ bundle exec ruby app.rb
Called: self.shutdown_all
[2017-12-12 14:21:56] INFO  WEBrick 1.3.1
[2017-12-12 14:21:56] INFO  ruby 2.4.2 (2017-09-14) [x86_64-darwin16]
== Sinatra (v2.0.0) has taken the stage on 4567 for development with backup from WEBrick
[2017-12-12 14:21:56] INFO  WEBrick::HTTPServer#start: pid=13174 port=4567

I'm on OSX 10.12.6, versions from Gemfile.lock

GEM
  remote: https://rubygems.org/
  specs:
    concurrent-ruby (1.0.5)
    mustermann (1.0.1)
    rack (2.0.3)
    rack-protection (2.0.0)
      rack
    sinatra (2.0.0)
      mustermann (~> 1.0)
      rack (~> 2.0)
      rack-protection (= 2.0.0)
      tilt (~> 2.0)
    sucker_punch (2.0.4)
      concurrent-ruby (~> 1.0.0)
    tilt (2.0.8)

PLATFORMS
  ruby

DEPENDENCIES
  sinatra
  sucker_punch

BUNDLED WITH
   1.16.0