Allow extra runtime deps to be defined when using prune_bundler

[daveallie]

Extra gems are sometimes needed in the before_fork block or before Bundler gets loaded on the workers. This PR adds the ability to define any extra bundler gems to load when pruning bundler.

As an example, when using https://github.com/schneems/puma_worker_killer, it should be loaded in the before_fork of the config/puma.rb file like so:

before_fork do
  require 'puma_worker_killer'

  PumaWorkerKiller.config do |config|
    config.ram           = 1024
    config.frequency     = 5
    config.percent_usage = 0.98
    config.reaper_status_logs = false
  end
  PumaWorkerKiller.start
end

If you have installed the gem from the github repo, and are using the prune_bundler option, the require 'puma_worker_killer' will fail. The only way to get it to run is to require bundler, and then require the gem in the context of bundler. Which, if you're also attempting to prune bundler, completely defeats the purpose and means gems aren't reloaded across phased restarts.

[daveallie]

Upon further testing it looks like this won't worker for bundler gem installed from git as https://github.com/puma/puma/blob/master/bin/puma-wild#L20 calls gem 'gemname', 'gemversion' which looks for rubygem gems.

Maybe the best way forward is to resolve needed gem paths before forking (at the top of config/puma.rb), and then adding them to the load path after fork?

[daveallie]

Not running the extra dependencies through the gem call alleviates my previous concern, adding the path to $LOAD_PATH is enough.

[nateberkopec]

Paging @schneems

[schneems]

Hi, I wrote puma worker killer. Does it work if in a before_block ? zombocom/puma_worker_killer#43

I've not used prune_bundler before. Sounds like we want to do it if you're updating your gems so that bundler will update your load path. I would think that we want this to work with all git dependencies out of the box and not just special ones that we define. Does bundler have any ways to "refresh" a load path from an updated Gemfile?

[daveallie]

prune_bundler runs before the before_fork block. It unloads all gems, except puma iirc, so that during your app boot, you can get bundler's context and load all the gems back in. I believe it is done this way so that individual workers hold the bundler context, allowing a rolling restart to reload all gems (and potentially use new ones if the gemfile has changed).

Before submitting it this way I attempted to load puma worker killer using bundler in the before_fork block, but to do it I needed to load bundler as well (as I was using a git ref for the gem). This meant that app workers were booted with a provided bundler context, meaning that rolling restarts didn't reload existing gems, unload old ones, or reload new ones.

I could have copy-pasted the logic behind prune_bundler into the top of my before_fork but thought it better to loop it into the existing logic and make it available for anyone else in the same situation.

[daveallie]

Any update on this PR?

[Dombo]

Hi, we've encountered a usecase for this inside one of our production applications - I notice that CI is failing on unrelated tests to this PR, has any of the maintainers had a chance to review the comment made by @daveallie on 07/12/2016.

Thanks

[nateberkopec]

@Dombo can you expand on your use-case? Same as @daveallie's?

[Dombo]

@nateberkopec sorry if I wasn't clear but yes it's the same use case as @daveallie's

[daveallie]

@nateberkopec any chance of moving this PR forward? It's almost been a year and nothing has really happened.

[daveallie]

Hi @nateberkopec or @schneems. Just thought I'd ping this again. Every time I want to use Puma master I need to rebase these changes on top.

puma_worker_killer being required without the use of bundler means that it will fail to find puma if puma is being sourced through a github ref, meaning the app crashes on boot. Happy to work around this problem if you have another way, but at the moment it makes the most sense to me to allow user defined gems to be included required in the master process's memory.

[nateberkopec]

This is always true.

[nateberkopec]

Rather than a rescue nil, we should do a proper begin/rescue here, which will get rid of the the if/else.

[daveallie]

There's actually no need for a rescue here at all, I don't know what I was thinking https://www.rubydoc.info/github/bundler/bundler/Bundler/RubygemsIntegration#loaded_specs-instance_method, loaded_specs is just a hash lookup. 🤦‍♂️

[nateberkopec]

"Couldn't to"

[nateberkopec]

"each gem name passed"

[nateberkopec]

Can people accept a Bundler version requirement for this feature? It's currently bugged because the require paths are wrong. I'd rather just use bundler's require_full_paths method but it's bundler 1.15+.

gemspec.require_paths returns mixed absolute and relative paths for me on 1.17.2: ["/Users/nateberkopec/.gem/ruby/2.6.3/extensions/x86_64-darwin-18/2.6.0-static/puma-3.12.1", "lib"], which breaks the entire prune process.

[nateberkopec]

Also there's no way we can merge this without tests.

[nateberkopec]

Here's some ideas for refactoring this method while we're here. https://gist.github.com/nateberkopec/386db42c0eb3fe4fa91b009dd9f1f84b

[daveallie]

Apologies for the sloppy nature of this PR, will bring it up to scratch

[daveallie]

So rubygem's full_require_paths didn't exist until rubygems v2.2.0 which isn't required by bundler until v2. Setting a bundler requirement of >= 2 seems a little extreme for this. We could do something similar to Bundler.rubygems.loaded_gem_paths and utilise full_require_paths if it's available, otherwise fallback to attempting to build full paths ourselves