mongrel unicorn - Jun 2010 - unicorn_rails cleanup (possible fix for Rails3) pushed

Hi all,

I''ve pushed the following patch out go git://git.bogomips.org/unicorn
along with a few other Rails-related test updates.  This is more of a
shotgun fix (but less code is better :) since I haven''t been able to
reproduce the brokeness people have been seeing with "unicorn_rails"
and Rails 3 betas.

Even though "unicorn" works perfectly well for Rails3, some folks will
inevitably run "unicorn_rails" because of the "rails" in its
name.

If I were to do it all over again, I would''ve just made everybody write
config.ru files for Rails.  But yes, programming is like sex, make one
mistake and support it for life :)

You can grab a git gem here, too:

  http://unicorn.bogomips.org/files/unicorn-0.99.0.16.g59a625.gem
>From 4b44e21957e4cb8ec6ace5604fbe096dfd8959d2 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson at yhbt.net>
Date: Thu, 3 Jun 2010 22:52:11 +0000
Subject: [PATCH] unicorn_rails: avoid duplicating config.ru logic

This should allow "unicorn_rails" to be used seamlessly
with Rails 3 projects which package config.ru for you.
---
 bin/unicorn_rails |   50 ++++++++++++++------------------------------------
 1 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/bin/unicorn_rails b/bin/unicorn_rails
index 72ab288..45a9b11 100755
--- a/bin/unicorn_rails
+++ b/bin/unicorn_rails
@@ -109,53 +109,30 @@ end
 
 ru = ARGV[0] || (File.exist?(''config.ru'') ?
''config.ru'' : nil)
 
-if ru && ru =~ /\.ru\z/
-  # parse embedded command-line options in config.ru comments
-  /^#\\(.*)/ =~ File.read(ru) and opts.parse!($1.split(/\s+/))
-end
-
-def rails_builder(ru, daemonize)
+def rails_builder(daemonize)
   # this lambda won''t run until after forking if preload_app is false
   lambda do ||
     # Load Rails and (possibly) the private version of Rack it bundles.
     begin
       require ''config/boot''
+      require ''config/environment''
     rescue LoadError => err
       abort "#$0 must be run inside RAILS_ROOT: #{err.inspect}"
     end
 
-    inner_app = case ru
-    when nil
-      require ''config/environment''
-
-      defined?(::Rails::VERSION::STRING) or
-        abort "Rails::VERSION::STRING not defined by
config/{boot,environment}"
-      # it seems Rails >=2.2 support Rack, but only >=2.3 requires it
-      old_rails = case ::Rails::VERSION::MAJOR
-      when 0, 1 then true
-      when 2 then Rails::VERSION::MINOR < 3 ? true : false
-      else
-        false
-      end
-
-      if old_rails
-        require ''unicorn/app/old_rails''
-        Unicorn::App::OldRails.new
-      else
-        ActionController::Dispatcher.new
-      end
-    when /\.ru$/
-      raw = File.read(ru)
-      raw.sub!(/^__END__\n.*/, '''')
-      eval("Rack::Builder.new {(#{raw}\n)}.to_app", TOPLEVEL_BINDING,
ru)
+    defined?(::Rails::VERSION::STRING) or
+      abort "Rails::VERSION::STRING not defined by
config/{boot,environment}"
+    # it seems Rails >=2.2 support Rack, but only >=2.3 requires it
+    old_rails = case ::Rails::VERSION::MAJOR
+    when 0, 1 then true
+    when 2 then Rails::VERSION::MINOR < 3 ? true : false
     else
-      require ru
-      Object.const_get(File.basename(ru, ''.rb'').capitalize)
+      false
     end
 
     Rack::Builder.new do
       map_path = ENV[''RAILS_RELATIVE_URL_ROOT''] ||
''/''
-      if inner_app.class.to_s == "Unicorn::App::OldRails"
+      if old_rails
         if map_path != ''/''
           # patches + tests welcome, but I really cbf to deal with this
           # since all apps I''ve ever dealt with just use "/"
...
@@ -163,23 +140,24 @@ def rails_builder(ru, daemonize)
         end
         $stderr.puts "LogTailer not available for Rails < 2.3"
unless daemonize
         $stderr.puts "Debugger not available" if $DEBUG
+        require ''unicorn/app/old_rails''
         map(map_path) do
           use Unicorn::App::OldRails::Static
-          run inner_app
+          run Unicorn::App::OldRails.new
         end
       else
         use Rails::Rack::LogTailer unless daemonize
         use Rails::Rack::Debugger if $DEBUG
         map(map_path) do
           use Rails::Rack::Static
-          run inner_app
+          run ActionController::Dispatcher.new
         end
       end
     end.to_app
   end
 end
 
-app = rails_builder(ru, daemonize)
+app = ru ? Unicorn.builder(ru, opts) : rails_builder(daemonize)
 options[:listeners] << "#{host}:#{port}" if set_listener
 
 if $DEBUG
-- 
Eric Wong

On Thu, Jun 3, 2010 at 9:58 PM, Eric Wong <normalperson at yhbt.net>
wrote:
<snip>
> If I were to do it all over again, I would''ve just made everybody
write
> config.ru files for Rails. ?But yes, programming is like sex, make one
> mistake and support it for life :)
That is probably one of the funniest things I''ve read in awhile.
Thanks for the unexpected source of humor! :)

Best,
Michael Guterl

Michael Guterl <mguterl at gmail.com> wrote:
> On Thu, Jun 3, 2010 at 9:58 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> <snip>
> > If I were to do it all over again, I would''ve just made
everybody write
> > config.ru files for Rails. ?But yes, programming is like sex, make one
> > mistake and support it for life :)
> 
> That is probably one of the funniest things I''ve read in awhile.
> Thanks for the unexpected source of humor! :)
You''re welcome :>  It''s not my line, but I heard it many
years ago.

-- 
Eric Wong

Eric Wong <normalperson at yhbt.net> wrote:
> Hi all,
> 
> I''ve pushed the following patch out go
git://git.bogomips.org/unicorn
> along with a few other Rails-related test updates.  This is more of a
> shotgun fix (but less code is better :) since I haven''t been able
to
> reproduce the brokeness people have been seeing with
"unicorn_rails"
> and Rails 3 betas.
<snip>
> You can grab a git gem here, too:
> 
>   http://unicorn.bogomips.org/files/unicorn-0.99.0.16.g59a625.gem
Anybody get a chance to try this?

-- 
Eric Wong

Hi Eric.

It would appear that recent Rails 3 changes have broken unicorn_rails,
just like they broke Phusion Passenger. Here''s a patch which fixes the
problem.
http://gist.github.com/429944

With kind regards,
Hongli Lai

-- 
Phusion | The Computer Science Company

Web: http://www.phusion.nl/
E-mail: info at phusion.nl
Chamber of commerce no: 08173483 (The Netherlands)

Hongli Lai <hongli at phusion.nl> wrote:
> Hi Eric.
> 
> It would appear that recent Rails 3 changes have broken unicorn_rails,
> just like they broke Phusion Passenger. Here''s a patch which fixes
the
> problem.
> http://gist.github.com/429944
Thanks Hongli, so this only affects people who remove the
config.ru that Rails 3 creates for them?  Yikes...

A few comments:

+  if ::Rails::VERSION::MAJOR >= 3 &&
::File.exist?(''config/application.rb'')
+    ::File.read(''config/application.rb'') =~ /^module (.+)$/

Maybe a more flexible regexp like this: /^module\s+([\w:]+)\s*$/ (or
maybe even starting with: "^\s*") would be more reliable for folks who
leave extra spaces around.

Unfortunately, Ruby is not Python :)

@@ -148,9 +167,9 @@ def rails_builder(daemonize)
       else
         use Rails::Rack::LogTailer unless daemonize
         use Rails::Rack::Debugger if $DEBUG
+        use Rails::Rack::Static, map_path
         map(map_path) do
-          use Rails::Rack::Static
-          run ActionController::Dispatcher.new
+          run rails_dispatcher

Changing the call to use Rails::Rack::Static there is wrong.  map_path
is the URI prefix (RAILS_RELATIVE_URL_ROOT) and not the static file
path we serve from.  It appears the deprecation in Rails 3 broke some
things and ActionDispatch::Static is configured slightly differently.

Let me know if the edited patch below looks alright to you.

I''ll also push out a few Rails 3 tests that exercise the missing
config.ru cases.
>From 222ae0a353eda446a480e5c4b473a218304f9594 Mon Sep 17 00:00:00 2001
From: Hongli Lai (Phusion) <hongli at phusion.nl>
Date: Tue, 8 Jun 2010 13:22:25 +0200
Subject: [PATCH] Fix unicorn_rails compatibility with the latest Rails 3 code

This allows us to properly detect Rails 3 installations
in cases where config.ru is not present.

[ew: expanded commit message
 fixed static file serving,
 more flexible regexp for matching module ]

ref: mid.gmane.org/AANLkTiksBxIo_PFWoiPTWi1entXZRb7D2uE-Rl7H3lbw at
mail.gmail.com
Acked-by: Eric Wong <normalperson at yhbt.net>
---
 bin/unicorn_rails |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/bin/unicorn_rails b/bin/unicorn_rails
index 45a9b11..ed235ba 100755
--- a/bin/unicorn_rails
+++ b/bin/unicorn_rails
@@ -109,6 +109,24 @@ end
 
 ru = ARGV[0] || (File.exist?(''config.ru'') ?
''config.ru'' : nil)
 
+def rails_dispatcher
+  if ::Rails::VERSION::MAJOR >= 3 &&
::File.exist?(''config/application.rb'')
+    if ::File.read(''config/application.rb'') =~
/^module\s+([\w:]+)\s*$/
+      app_module = Object.const_get($1)
+      begin
+        result = app_module::Application
+      rescue NameError
+      end
+    end
+  end
+
+  if result.nil? && defined?(ActionController::Dispatcher)
+    result = ActionController::Dispatcher.new
+  end
+
+  result || abort("Unable to locate the application dispatcher
class")
+end
+
 def rails_builder(daemonize)
   # this lambda won''t run until after forking if preload_app is false
   lambda do ||
@@ -149,8 +167,12 @@ def rails_builder(daemonize)
         use Rails::Rack::LogTailer unless daemonize
         use Rails::Rack::Debugger if $DEBUG
         map(map_path) do
-          use Rails::Rack::Static
-          run ActionController::Dispatcher.new
+          if defined?(ActionDispatch::Static)
+            use ActionDispatch::Static, "#{Rails.root}/public"
+          else
+            use Rails::Rack::Static
+          end
+          run rails_dispatcher
         end
       end
     end.to_app
-- 
Eric Wong

On Tue, Jun 8, 2010 at 9:20 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> Thanks Hongli, so this only affects people who remove the
> config.ru that Rails 3 creates for them? ?Yikes...
No. The problem even occurs if you already have config.ru. But the
thing is, Rails 3 has deprecated ActionController::Dispatcher a few
weeks ago and replaced it with a stub. Rails::Rack::Static changed its
interface and must be constructed differently. The only way to obtain
a valid Rack endpoint for the app seems to be parsing
config/application.rb

> A few comments:
>
> + ?if ::Rails::VERSION::MAJOR >= 3 &&
::File.exist?(''config/application.rb'')
> + ? ?::File.read(''config/application.rb'') =~ /^module
(.+)$/
>
> Maybe a more flexible regexp like this: /^module\s+([\w:]+)\s*$/ (or
> maybe even starting with: "^\s*") would be more reliable for
folks who
> leave extra spaces around.
I think it''s easier to just call ''strip''. :)
> Unfortunately, Ruby is not Python :)
>
> @@ -148,9 +167,9 @@ def rails_builder(daemonize)
> ? ? ? else
> ? ? ? ? use Rails::Rack::LogTailer unless daemonize
> ? ? ? ? use Rails::Rack::Debugger if $DEBUG
> + ? ? ? ?use Rails::Rack::Static, map_path
> ? ? ? ? map(map_path) do
> - ? ? ? ? ?use Rails::Rack::Static
> - ? ? ? ? ?run ActionController::Dispatcher.new
> + ? ? ? ? ?run rails_dispatcher
>
> Changing the call to use Rails::Rack::Static there is wrong. ?map_path
> is the URI prefix (RAILS_RELATIVE_URL_ROOT) and not the static file
> path we serve from. ?It appears the deprecation in Rails 3 broke some
> things and ActionDispatch::Static is configured slightly differently.
>
> Let me know if the edited patch below looks alright to you.
Yes it looks fine. A bit overcomplicated regexp compared to using
''strip'' but whatever works. :)

With kind regards,
Hongli Lai
-- 
Phusion | The Computer Science Company

Web: http://www.phusion.nl/
E-mail: info at phusion.nl
Chamber of commerce no: 08173483 (The Netherlands)

Hongli Lai <hongli at phusion.nl> wrote:
> On Tue, Jun 8, 2010 at 9:20 PM, Eric Wong <normalperson at yhbt.net>
wrote:
> > Thanks Hongli, so this only affects people who remove the
> > config.ru that Rails 3 creates for them? ?Yikes...
> 
> No. The problem even occurs if you already have config.ru. But the
> thing is, Rails 3 has deprecated ActionController::Dispatcher a few
> weeks ago and replaced it with a stub. Rails::Rack::Static changed its
> interface and must be constructed differently. The only way to obtain
> a valid Rack endpoint for the app seems to be parsing
> config/application.rb
Actually, I made "unicorn_rails" completely bypass the
"rails_builder"
method if there''s a config.ru, so it should never hit the
ActionController::Dispatcher stuff.
> > Let me know if the edited patch below looks alright to you.
> 
> Yes it looks fine. A bit overcomplicated regexp compared to using
> ''strip'' but whatever works. :)
I just kept the regexp as-is, works for me.

I just managed to push this to git://git.bogomips.org/unicorn.git before
my Internet connection died on me earlier today.  I''ve beefed up the
tests a bit but will probably do more later.

Eric Wong (4):
      t0300: Rails 3 test actually uses unicorn_rails
      tests: libify common rails3 setup code
      unicorn_rails: fix requires for Ruby 1.9.2
      tests: add Rails 3 test for the missing config.ru case

Hongli Lai (Phusion) (1):
      Fix unicorn_rails compatibility with the latest Rails 3 code

Thanks again!

-- 
Eric Wong

Eric Wong <normalperson at yhbt.net> wrote:
> Hongli Lai <hongli at phusion.nl> wrote:
> > Hi Eric.
> > 
> > It would appear that recent Rails 3 changes have broken unicorn_rails,
> > just like they broke Phusion Passenger. Here''s a patch which
fixes the
> > problem.
> > http://gist.github.com/429944
> 
> Thanks Hongli, so this only affects people who remove the
> config.ru that Rails 3 creates for them?  Yikes...
And for the completely bizzare: Rails 3 is ultra aggressive
when searching for config.ru, it walks up the directory tree,
even going all the way up to "/" if it can''t find config.ru.

I had a $HOME/config.ru leftover from a test on one of my boxes, and
usually have my working directory in $HOME/unicorn.   This was was
causing the unicorn_rails test for this (t0301) to fail because that
test relies on Rails /not/ being able to find config.ru.

So yes, don''t leave random artifacts lying around and love strace :)

-- 
Eric Wong