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

Conversation

@seven1m
Copy link
Contributor

seven1m commented Aug 8, 2019

Starting your server with BUNDLE_GEMFILE=Gemfile.rails6 would delete this environment variable on app restart when using --prune-bundler, when all you really want is to restore the variable to what it was before Bundler was activated.

I worked with @bensie on this and this PR should supersede #1794.

Starting your server with BUNDLE_GEMFILE=Gemfile.rails6 would delete this
environment variable on app restart when using `--prune-bundler`, when
all you really want is to restore the variable to what it was before
Bundler was activated.

I mimicked the existing code that preserves GEM_HOME.

Co-authored-by: James Miller <bensie@gmail.com>
test/test_integration.rb Outdated Show resolved Hide resolved
@nateberkopec nateberkopec added the feature label Aug 8, 2019
seven1m added 2 commits Aug 8, 2019
Test that the environment is not lost when doing a restart by using the
PATH env var as a canary. While we actually care about the
BUNDLER_GEMFILE env var, that one is harder to set up in testing since
it requires a full copy of Puma's Gemfile.

This change also has the benefit of not only protecting the
BUNDLE_GEMFILE var, but also the other variables that Bundler knows
about.
@seven1m seven1m changed the title Preserve BUNDLE_GEMFILE and add a test for it Preserve BUNDLE_GEMFILE (and other Bundler env vars) and add a test for it Aug 8, 2019
@nateberkopec
Copy link
Member

nateberkopec commented Aug 10, 2019

Hm, I wonder if we'll need to major version bump for this as people may have been relying on the old behavior of Bundle env variables getting wiped clean.

@bensie
Copy link
Contributor

bensie commented Aug 12, 2019

It's hard to imagine the scenario where you'd depend on that behavior, since there's no way to inject the Bundler variables you do want during the Puma reload and you're left with inconsistent state depending on whether you reload or restart.

But yeah it's up to you, I guess sometimes a bug fix is technically breaking 🤷‍♂

@nateberkopec
Copy link
Member

nateberkopec commented Aug 15, 2019

I think this is good to go, but I'm going to wait on merging it until it becomes more clear to me what the next 2-3 months of release schedule looks like.

If required immediately, I suggest using a fork until that time.

@nateberkopec nateberkopec added this to the 5.0.0 milestone Aug 15, 2019
test/test_integration.rb Outdated Show resolved Hide resolved
test/test_integration.rb Outdated Show resolved Hide resolved
@zw963
Copy link

zw963 commented Sep 20, 2019

thank you a lot, i will test this PR when merge into master.

@nateberkopec
Copy link
Member

nateberkopec commented Nov 11, 2019

OK, problem: for me with Bundler 1.17.2, this test passes with the original implementation.

I tried a couple of other env variables, everything is passing.

@seven1m
Copy link
Contributor Author

seven1m commented Nov 14, 2019

Ahhh! You're completely right. I don't even know what happened, but I really thought I did this right. I know this one-line change fixes issues in our applications -- we use this patch in production to allow us to reload our apps while keeping our environment variables from getting wiped out.

I really struggled to write a good test to prove this, and I guess I didn't even succeed in writing a proper test. 🤦‍♂

I'm open to suggestions!

If you're ok with it, can we leave this open and I'll work on fixing the test...?

@nateberkopec
Copy link
Member

nateberkopec commented Nov 14, 2019

Oh of course we can leave it open. If it fixes something then the state is changed somehow, we just need to document what that is with a test.

@dentarg
Copy link
Contributor

dentarg commented Nov 23, 2019

@nateberkopec @seven1m I think the test need to test with an BUNDLE_ environment variable, it is only those that are wiped, see the the clean_env implementation in Bundler (it is the same in 1.17.2 and 2.0.2)

(In Bundler 2.1.0 that method has been renamed to unbundled_env)

@nateberkopec
Copy link
Member

nateberkopec commented Nov 26, 2019

I think the test need to test with an BUNDLE_ environment variable

Pretty sure I tried that and it still passed with original implementation, but now I can't remember.

rokumatsumoto added a commit to rokumatsumoto/boyutluseyler that referenced this pull request Dec 9, 2019
- Cannot boot puma workers

from puma.log on AWS EB
--
=== puma startup: 2019-12-09 12:47:04 +0000 ===
=== puma startup: 2019-12-09 12:47:04 +0000 ===
[19558] Early termination of worker
[19600] Early termination of worker
[19619] Early termination of worker
[19635] Early termination of worker
[19640] Early termination of worker
[19646] Early termination of worker
[19654] Early termination of worker
[19659] Early termination of worker

puma/puma#2018
puma/puma#1893
@nateberkopec
Copy link
Member

nateberkopec commented Feb 12, 2020

@seven1m Did you see @dentarg's comment? It's possible that would help get this test working correctly.

We need a test that's not passing on master for this to work. Maybe #2018 will help us to figure out what needs to be tested here?

@seven1m
Copy link
Contributor Author

seven1m commented Feb 12, 2020

Thanks for the tip, I'll take a look at this and see what I can do today.

seven1m added 3 commits Feb 12, 2020
Starting your server with BUNDLE_GEMFILE=Gemfile.rails6 would delete this
environment variable on app restart when using --prune-bundler, when all
you really want is to restore the variable to what it was before Bundler
was activated.

Writing a test for this proved to be difficult because the bug only seems
to manifest itself when running the server under the following conditions:

- Using `bundle exec` to run the puma command
- Using a non-zero number of workers, e.g. `-w 2`
- Setting `BUNDLE_GEMFILE` to something other than "Gemfile"
@seven1m
Copy link
Contributor Author

seven1m commented Feb 13, 2020

Update: I got all the builds to pass, but I'm nervous it's the wrong answer. 😬

I'm still working on it...

@bensie
Copy link
Contributor

bensie commented Feb 13, 2020

🍿

@nateberkopec if the outstanding issue is really just Ruby 2.2-specific and you're thinking this PR can only be merged for Puma 5.x, any shot of removing support for 2.2 then? As of March 2018, 2.2 is EOL.

@nateberkopec
Copy link
Member

nateberkopec commented Feb 13, 2020

We base Ruby version support on usage, not on EOL, so there’s a bit of lag there.

seven1m added 4 commits Feb 13, 2020
...and go back to using Bundler.with_clean_env.
@seven1m seven1m changed the title Preserve BUNDLE_GEMFILE (and other Bundler env vars) and add a test for it Preserve BUNDLE_GEMFILE and add a test for it Feb 13, 2020
@@ -297,8 +297,10 @@ def prune_bundler

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

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Feb 14, 2020

Member

Oh, what happened?

This comment has been minimized.

Copy link
@seven1m

seven1m Feb 14, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@AlexWayfer

AlexWayfer Feb 20, 2020

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.

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Feb 20, 2020

Member

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

This comment has been minimized.

Copy link
@AlexWayfer

AlexWayfer Feb 20, 2020

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.

This comment has been minimized.

Copy link
@dentarg

dentarg Feb 20, 2020

Contributor

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)

This comment has been minimized.

Copy link
@seven1m

seven1m Feb 21, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@dentarg

dentarg Feb 29, 2020

Contributor

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

@nateberkopec
Copy link
Member

nateberkopec commented Feb 17, 2020

Gonna test this again locally against master, but from the code perspective it LGTM.

History.md Outdated Show resolved Hide resolved
@nateberkopec
Copy link
Member

nateberkopec commented Feb 18, 2020

Fails on master! One more change to the changelog and then we hit the big green button.

@nateberkopec nateberkopec merged commit 8c9b3eb into puma:master Feb 18, 2020
20 checks passed
20 checks passed
build
Details
ubuntu-18.04, 2.3
Details
ubuntu-18.04, 2.4
Details
ubuntu-18.04, 2.5
Details
ubuntu-18.04, 2.6
Details
ubuntu-18.04, 2.7
Details
ubuntu-18.04, ruby-head
Details
macos, 2.3
Details
macos, 2.4
Details
macos, 2.5
Details
macos, 2.6
Details
macos, 2.7
Details
macos, ruby-head
Details
windows-latest, 2.3
Details
windows-latest, 2.4
Details
windows-latest, 2.5
Details
windows-latest, 2.6
Details
windows-latest, 2.7
Details
windows-latest, ruby-head
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.