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

Preserve BUNDLE_GEMFILE and add a test for it #1893

Merged
merged 29 commits into from Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
56bbc2d
Preserve BUNDLE_GEMFILE and add a test for it
seven1m Aug 8, 2019
a3ed997
Go back to using Bundler.with_original_env
seven1m Aug 8, 2019
4b27762
Gemfile.duplicate is no longer a thing
seven1m Aug 8, 2019
dad3e58
Rename test
seven1m Aug 8, 2019
9c2dd28
Skip this test on Windows
seven1m Aug 19, 2019
5e7a6fd
No need to fork
seven1m Aug 19, 2019
7d6780d
Merge remote-tracking branch 'upstream/master'
seven1m Feb 12, 2020
4cfd3b8
Preserve BUNDLE_GEMFILE and other Bundler env vars
seven1m Feb 12, 2020
242c140
Remove unneeded test support file
seven1m Feb 12, 2020
f15121f
Update changelog
seven1m Feb 12, 2020
3020316
Run test command without bash
seven1m Feb 13, 2020
d89af85
Remove skip for USR2
seven1m Feb 13, 2020
c83401c
Move test into separate file
seven1m Feb 13, 2020
cbf32d5
Add some logging to help diagnose Travis build failure
seven1m Feb 13, 2020
95bcd05
Is that really running over and over???
seven1m Feb 13, 2020
3be5f32
Yes! It is, but why?
seven1m Feb 13, 2020
9dcd2e3
Don't try to prune_bundler again
seven1m Feb 13, 2020
2ed158e
OMG it works! Remove diagnostic logging.
seven1m Feb 13, 2020
359cb69
When is Bundler loaded by puma-wild?
seven1m Feb 13, 2020
08dc10f
Reenable logging in the test
seven1m Feb 13, 2020
0d020df
Is Bundler defined when using with_clean_env?
seven1m Feb 13, 2020
d804495
When is Bundler loaded? (this is nuts)
seven1m Feb 13, 2020
10d4183
Let's see them args!
seven1m Feb 13, 2020
d84e0ae
Now let's see the ENV!
seven1m Feb 13, 2020
3d1c5e0
Bundler wins. Just set BUNDLE_GEMFILE manually
seven1m Feb 13, 2020
f73cd57
Remove diagnostic logging
seven1m Feb 13, 2020
bc4217b
Update changelog
seven1m Feb 13, 2020
62f075f
Add back test skip for platforms without USR2
seven1m Feb 14, 2020
1846ef3
Clarify changelog entry
seven1m Feb 18, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions History.md
Expand Up @@ -10,6 +10,7 @@
* Bugfixes
* Your bugfix goes here (#Github Number)
* Windows update extconf.rb for use with ssp and varied Ruby/MSYS2 combinations (#2069)
* Preserve `BUNDLE_GEMFILE` env var on restart (#1893)
seven1m marked this conversation as resolved.
Show resolved Hide resolved

* Refactor
* Remove unused loader argument from Plugin initializer (#2095)
Expand Down
2 changes: 2 additions & 0 deletions lib/puma/launcher.rb
Expand Up @@ -297,8 +297,10 @@ def prune_bundler

log '* Pruning Bundler environment'
home = ENV['GEM_HOME']
bundle_gemfile = ENV['BUNDLE_GEMFILE']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, what happened?

Copy link
Contributor Author

@seven1m seven1m Feb 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about the environment in the Travis build was causing Bundler.with_original_env to make Puma load in an endless loop. Puma would load, it would call Bundler.with_original_env, then call Kernel.exec('puma-wild', ...), then the loop would start over again.

You can see that loop in this failed build.

I could not reproduce this issue locally, even with the same version of Ubuntu, same version of Ruby, same version of Bundler, and same version of RVM. There are probably other pieces in the environment on the Travis build that I am not aware of and did not fully replicate.

Testing this required, as you can see, pushing many commits to track down the problem.

The prune_bundler method is recursive, as you know, and it relies on the line return unless defined?(Bundler) as its terminal case. Something about the Travis environment (I suspect it's RVM) was causing Bundler to be loaded every time the method recursed.

As I am out of time to work on this, I went back to the hacky solution that copies the needed env var (BUNDLE_GEMFILE) back into the environment after we run Bundler.with_clean_env.

It's not my favorite solution, at all. But it meets our needs and should not break anything in other people's environments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not reproduce this issue locally, even with the same version of Ubuntu, same version of Ruby, same version of Bundler, and same version of RVM. There are probably other pieces in the environment on the Travis build that I am not aware of and did not fully replicate.

Travis is pretty dirty in my experience, so I've switched my repos to other CIs. You can consider something similar too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've moved almost everything to Github Actions, but Ruby 2.2 can't be moved (yet?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ruby 2.2 is no more maintained, even security, but I understand that you want to support its users. But I personally consider its usage dangerous and prefer to drop support from gems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since https://github.com/ruby/setup-ruby/releases/tag/v1.14.0 it is possible to use Ruby 2.2 on GitHub Actions... but it could be that there's something else missing for the Puma test setup (e.g. that version wasn't built with the OpenSSL version we want to test Puma with, I'm sure @MSP-Greg knows about this and maybe even have tried it, I don't have the time to double check it now)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note that I believe Travis' environment actually saved us from shipping a bug here. Something in that particular environment was causing the Bundler constant to be loaded earlier than anticipated. I suspect it is some RVM magic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difference between Travis and GitHub Actions is (was) the bundle install command

Travis does the following (https://travis-ci.org/puma/puma/jobs/656438353#L885)

bundle install --jobs=3 --retry=3 --path=${BUNDLE_PATH:-vendor/bundle}

Before #2126 (5 days ago), --path wasn't used on GitHub Actions, now it is:

- name: bundle install
run: bundle install --jobs 4 --retry 3 --path=.bundle/puma

Haven't checked if it matters for this particular discussion but thought it was worth mentioning here

Bundler.with_clean_env do
ENV['GEM_HOME'] = home
ENV['BUNDLE_GEMFILE'] = bundle_gemfile
ENV['PUMA_BUNDLER_PRUNED'] = '1'
args = [Gem.ruby, puma_wild_location, '-I', dirs.join(':'), deps.join(',')] + @original_argv
# Ruby 2.0+ defaults to true which breaks socket activation
Expand Down
1 change: 1 addition & 0 deletions test/bundle_preservation_test/.gitignore
@@ -0,0 +1 @@
Gemfile.bundle_env_preservation_test.lock
@@ -0,0 +1 @@
gem 'puma', path: '../..'
1 change: 1 addition & 0 deletions test/bundle_preservation_test/config.ru
@@ -0,0 +1 @@
run lambda { |env| [200, {'Content-Type'=>'text/plain'}, [ENV['BUNDLE_GEMFILE'].inspect]] }
18 changes: 14 additions & 4 deletions test/helpers/integration.rb
Expand Up @@ -75,14 +75,24 @@ def restart_server_and_listen(argv)
end

# reuses an existing connection to make sure that works
def restart_server(connection)
def restart_server(connection, log: false)
Process.kill :USR2, @pid
connection.write "GET / HTTP/1.1\r\n\r\n" # trigger it to start by sending a new request
wait_for_server_to_boot
wait_for_server_to_boot(log: log)
end

def wait_for_server_to_boot
true while @server.gets !~ /Ctrl-C/ # wait for server to say it booted
# wait for server to say it booted
def wait_for_server_to_boot(log: false)
if log
puts "Waiting for server to boot..."
begin
line = @server.gets
puts line if line && line.strip != ''
end while line !~ /Ctrl-C/
puts "Server booted!"
else
true while @server.gets !~ /Ctrl-C/
end
end

def connect(path = nil, unix: false)
Expand Down
35 changes: 35 additions & 0 deletions test/test_preserve_bundler_env.rb
@@ -0,0 +1,35 @@
require_relative "helper"
require_relative "helpers/integration"

class TestPreserveBundlerEnv < TestIntegration
def setup
skip NO_FORK_MSG unless HAS_FORK
super
end

# It does not wipe out BUNDLE_GEMFILE et al
def test_usr2_restart_preserves_bundler_environment
seven1m marked this conversation as resolved.
Show resolved Hide resolved
skip_unless_signal_exist? :USR2

@tcp_port = UniquePort.call
env = {
# Intentionally set this to something we wish to keep intact on restarts
"BUNDLE_GEMFILE" => "Gemfile.bundle_env_preservation_test",
# Don't allow our (rake test's) original env to interfere with the child process
"BUNDLER_ORIG_BUNDLE_GEMFILE" => nil
}
# Must use `bundle exec puma` here, because otherwise Bundler may not be defined, which is required to trigger the bug
cmd = "bundle exec puma -q -w 1 --prune-bundler -b tcp://#{HOST}:#{@tcp_port}"
Dir.chdir(File.expand_path("bundle_preservation_test", __dir__)) do
@server = IO.popen(env, cmd.split, "r")
end
wait_for_server_to_boot
@pid = @server.pid
connection = connect
initial_reply = read_body(connection)
assert_match("Gemfile.bundle_env_preservation_test", initial_reply)
restart_server connection
new_reply = read_body(connection)
assert_match("Gemfile.bundle_env_preservation_test", new_reply)
end
end