Prevent Rack config script scope leakage, ensure methods are sent to Rack::Builder. #27

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

mildmojo commented Jul 9, 2012

This request addresses issue #26.

Nack::Builder#method_missing misses calls to Rack::Builder methods when their names collide with methods defined in scopes upstream of Nack::Builder. I've added a test and fixture to show the problem, removed nack/builder.rb, and converted Nack::Server to use Rack::Builder directly.

I hit this bug when one of my Rails apps failed to load in Pow.

I haven't included the rubygems fallback when Rack is required in nack/server.rb. We can totally change this around if it's important. I'm interested to know the reason why Nack::Builder was originally added, too.

Owner
josh commented Jul 9, 2012

Sorry, this breaks all lazy loading of rack. Eagering loading can cause gem conflict errors since nack boots before bundler is invoked.

@josh josh closed this Jul 9, 2012
mildmojo commented Jul 9, 2012

Ah, cool. I'll take another crack at it. Do you have any advice?

If I understand you correctly, when the app's config.ru is sent to Nack::Builder, you're waiting for the first method call that trips method_missing to load Rack::Builder, expecting that the missed method belongs to Rack::Builder. You do this so that the startup script can resolve its own require chain before you require Rack::Builder?

When is this an issue? Rack itself doesn't behave this way. If you're calling rackup to start your app, Rack::Server.start definitely loads Rack and Rack::Builder before config.ru starts up. What's different about the way you're using Rack? If there was a specific situation you found where this was necessary, could you please describe it?

Owner
josh commented Jul 10, 2012

pow/nack starts up outside the context of bundler. So it doesn't know what version of rack your app actually needs. require 'rack' will load the latest version by the default, which can be wrong.

If you use rackup, you're supposed to use bundle exec rackup which loads bundler first.

Makes sense; thanks. So Nack needs to be bundler-aware and load the rack version referenced in the app's gem list, if any. I can dig it.

I think the fix is just a combination of using Rack::Builder directly and adding bundler magic to nack_worker. I'll give it a shot.

Owner
josh commented Jul 10, 2012

nack should not know anything about the existence of bundler

Nack already knows about bundler--it's the reason you added Nack::Builder to lazy-load Rack::Builder in the first place, right? I think it either needs more complete bundler support, or it doesn't need to lazy-load Rack::Bundler.

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