Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prune bundler causes rack to be loaded twice #859

Closed
boxofrad opened this issue Jan 15, 2016 · 12 comments
Closed

Prune bundler causes rack to be loaded twice #859

boxofrad opened this issue Jan 15, 2016 · 12 comments

Comments

@boxofrad
Copy link

Howdy! 👋

I've been converting an app to use prune_bundler as recommended in DEPLOYMENT.md to ensure bundled gems are properly reloaded when restarting the server with USR1.

I noticed a lot of warnings in my logs caused by rack constants being redefined, for example:

/home/vagrant/development/puma-double-rack/vendor/ruby/2.1.0/gems/rack-1.6.4/lib/rack.rb:14: warning: already initialized constant Rack::VERSION
/var/lib/gems/2.1.0/gems/rack-1.6.4/lib/rack.rb:14: warning: previous definition of VERSION was here

After doing some investigation, it appears that rack is being loaded twice; once from my system-wide gems, and once from my app's vendor folder managed by bundler.

Because prune_bundler removes bundled gems from the $LOAD_PATH, it seems that this block (in configuration.rb) is loading the system-wide rack instead of the bundler rack:

# Load and use the normal Rack builder if we can, otherwise
# fallback to our minimal version.
def rack_builder
  begin
    require 'rack'
    require 'rack/builder'
  rescue LoadError
    # ok, use builtin version
    return Puma::Rack::Builder
  else
    return ::Rack::Builder
  end
end

Which makes sense... my understanding of this method is that it assumes that if require 'rack' succeeds it must be running under bundler, otherwise it uses Puma's own implementation of Rack::Builder.
However in my case as the gem is installed system-wide, the require will succeed when not under bundler.

While the obvious solution may be to simply remove the system-wide rack, I'm not sure that's the best solution because:

  1. In my particular case, it's not feasible to remove the system-wide rack.
  2. This behaviour breaks the isolation / safety contract provided by bundler, if a gem is bundled it should always be preferred over the system-wide gem.

Reproduction Steps

I've created a simple rack app that exhibits the behaviour:

https://github.com/boxofrad/puma-double-rack

Here are the conditions under which the behaviour can be observed:

  • One copy of rack is installed in GEM_HOME
  • Another copy of rack is installed by bundler
  • Bundler is configured to install gems to a path other than GEM_HOME (e.g. when installing with --deployment or --path mypath)
  • Puma is configured with prune_bundler

Thanks for all of your hard work on Puma! 💖

@boxofrad
Copy link
Author

My colleague pointed out that it may not be clear that the second require 'rack' is happening when we require 'sinatra' in our config.ru.

While I could remove the require 'rack' from the config.ru in my example app, I cannot remove the require 'sinatra' from my real app.

@boxofrad
Copy link
Author

Here's a very hacky workaround (in my puma.rb) that forces the bundler version into the $LOAD_PATH before the system-wide version:

if defined?(Bundler)
  prune_bundler
else
  require 'bundler'
  $LOAD_PATH.unshift File.join(Bundler.rubygems.find_name('rack').first.full_gem_path, 'lib')
end

boxofrad added a commit to boxofrad/puma that referenced this issue Jan 18, 2016
We cannot fully remove the dependency on Rack because gems such as Sinatra
monkey-patch their own extensions onto Rack (see puma#735). In those cases we
must use the real `Rack::Builder` instead of our own `Puma::Rack::Builder`.

We cannot determine the Rack path ahead of time and pass it to `puma-wild`
from the `#prune_bundler` method because it would prevent Rack refreshing
on restart.

Instead of shelling out to `bundle show` we could consider loading Bundler
and using it to determine the gem path, this would either require loading
'bundler/setup' which is undesirable as it would modify the `$LOAD_PATH`
in unexpected ways, or manually implementing just enough of `Bundler.setup`
ourselves, which would deeply couple Puma to the internals of Bundler.
boxofrad added a commit to boxofrad/puma that referenced this issue Jan 18, 2016
@evanphx
Copy link
Member

evanphx commented Jan 28, 2016

Thanks so much for the repo but it doesn't seem to cause the issue for me! Could you list the exact shell commands you run after cloning that repo to cause the issue?

@BRMatt
Copy link

BRMatt commented Jan 28, 2016

@evanphx If you run ./boot it should run puma in a way that reproduces the issue, is this not happening? (I work with Daniel) EDIT: From what I remember we were able to reproduce it using just that command. If that's not working we may need to try specifying the exact ruby/bundler versions.

@evanphx
Copy link
Member

evanphx commented Jan 28, 2016

Nope, it's not. Should I need to run any bundle commands?

@evanphx
Copy link
Member

evanphx commented Jan 28, 2016

Oh, I see that the vendor dir requires me to be running ruby 2.1.0. Let me install that and try again.

@BRMatt
Copy link

BRMatt commented Jan 28, 2016

Oh, I see that the vendor dir requires me to be running ruby 2.1.0. Let me install that and try again.

Doh! Sorry!

@evanphx
Copy link
Member

evanphx commented Jan 28, 2016

Ok, so I had to run bundle update puma to get the puma_http11 extension build properly. With that change, I ran ./boot and still don't get the double requires.

@evanphx
Copy link
Member

evanphx commented Jan 28, 2016

What version of bundler are you testing this with? Perhaps thats the variable that is keeping it from happening to me (which also means, if you could, update to the latest bundler and see if it still happens for you).

@evanphx
Copy link
Member

evanphx commented Jan 28, 2016

Ok! Got it! It's not that rack is installed into a different GEM_HOME, it's that it's installed into the ruby's default gem directory (i.e. gem install rack).

@boxofrad
Copy link
Author

Thanks so much @evanphx, sorry about the misinformation!

@BRMatt
Copy link

BRMatt commented Jan 28, 2016

@evanphx thanks for merging and the speedy release! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants