Rails::Server#app should be nice with Rack::URLMap #1516

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@guilleiguaran
Member

This a follow-up of #1331 (Actually I'm looking to solve this because I need it urgently).

I'm checking if super.instance exists before call it and call to super if it doesn't exists (for Rack::URLMap), I don't want remove entire method since according to 19c763f using it here reduces 2 methods call per request.

Also I do some changes to @andrew test to use a free port instead of port 3000 for server_tests. (Some people runs another Rails apps while run the tests ;)).

cc. @andrew @josevalim

@guilleiguaran
Member

Just in case, this the backtrace of the error:

~/code/MyThreeBest[master] % rails server
=> Booting WEBrick
=> Rails 3.1.0.rc1 application starting in development on http://0.0.0.0:3000
=> Call with -d to detach
=> Ctrl-C to shutdown server
Exiting
/Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/railties-3.1.0.rc1/lib/rails/commands/server.rb:46:in `app': undefined method `instance' for #<Rack::URLMap:0x000001051f0738> (NoMethodError)
    from /Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/rack-1.3.0/lib/rack/server.rb:301:in `wrapped_app'
    from /Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/rack-1.3.0/lib/rack/server.rb:252:in `start'
    from /Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/railties-3.1.0.rc1/lib/rails/commands/server.rb:70:in `start'
    from /Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/railties-3.1.0.rc1/lib/rails/commands.rb:54:in `block in <top (required)>'
    from /Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/railties-3.1.0.rc1/lib/rails/commands.rb:49:in `tap'
    from /Users/guille/.rvm/gems/ruby-1.9.2-p180@mythreebest/gems/railties-3.1.0.rc1/lib/rails/commands.rb:49:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'
@drogus
Member
drogus commented Jun 7, 2011

@guilleiguaran: can you change that fix to use to_app method that @josevalim proposed here: #1331 (comment) ?

@josevalim
Member

@guilleiguaran agreed. If you can make it call .to_app instead of .instance and define to_app in Rails::Application, I would merge it straight way.

@guilleiguaran
Member

@josevalim done, also I rebase with master.

One more thing: Looks like the server tests sometimes fail with "Errno::ECONNREFUSED: Connection refused - connect(2)" because are trying to get "/" before server finish booting, any ideas about how to solve it or we must delete it and revisit those tests later?

@josevalim josevalim commented on an outdated diff Jun 7, 2011
railties/lib/rails/application.rb
@@ -138,6 +138,10 @@ module Rails
@config ||= Application::Configuration.new(find_root_with_flag("config.ru", Dir.pwd))
end
+ def to_app
+ self.class.instance
@josevalim
josevalim Jun 7, 2011 Member

This is the same as self.

@andrew
Contributor
andrew commented Jun 7, 2011

https://github.com/rails/rails/pull/1516/files#L2R42 is supposed to stop this happening, similar to the server test in rack: https://github.com/rack/rack/blob/master/test/spec_server.rb#L65

It looks like the rails server goes to sleep and then walks up halfway through the boot process, making quite hard to know when its finally ready to serve pages, perhaps this is why the whole file is untested currently?

@josevalim
Member

Hrm, let's leave the tests out. Let's just have a fix that we can merge into 3-1-stable and master soon. I have also added an extra comment.

@guilleiguaran
Member

@josevalim Done, also I rebase all changes in a single commit :)

@josevalim
Member

Merged.

@josevalim josevalim closed this Jun 7, 2011
@josevalim
Member

Can you also provide a pull request for 3-1-stable?

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