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.

Sign up for GitHub

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

Jump to bottom

Fix Bundler::GemNotFound errors for nio4r gem #2427

Merged
merged 6 commits into from Oct 20, 2020
Merged

Fix Bundler::GemNotFound errors for nio4r gem #2427

merged 6 commits into from Oct 20, 2020

Conversation

cjlarose
Copy link
Member

@cjlarose cjlarose commented Oct 14, 2020 •
edited

Description

Fixes #2018

A fairly common and reasonable deployment strategy has been broken since the introduction of nio4r in puma 4.0. Imagine that a user is using puma in cluster mode. In order to deploy a new version of an application, a user's deployment tool does this:

  1. Add new version of the application's source to the server in a new directory
  2. Update the "current release" symlink to point to the new release directory (the symlink is configured as puma's directory in the config file)
  3. Issue a phased restart
  4. After verifying that the release was successful, delete the old version of the application from disk

During the next deployment, workers will fail to start. They'll encounter the error Could not find nio4r-2.5.1 in any of the sources (Bundler::GemNotFound). The root cause of the issue is described in rubygems/rubygems#4004. As of the time of writing, that issue is not yet addressed. This PR introduces a workaround into puma that essentially sidesteps the Bundler issue.

The proposed change essentially just removes nio4r from being activated in the puma master process for cluster-mode puma as long as you're using prune_bundler (which is basically required anyway if you want phased restarts). nio4r is used for doing async IO in Puma::Reactor, and since in cluster-mode puma the master process doesn't actually run an instance of Puma::Reactor, we can defer loading nio4r until we're in the worker process.

In addition to adding tests in puma itself for this change, I also verified that the repro in https://github.com/cjlarose/puma-phased-restart-could-not-find-gem-errors is fixed by the same change.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@cjlarose
Update extra_runtime_dependencies test to test master proccess's gems
7faa456 
The `extra_runtime_dependencies` option allows one to activate gems in
the puma master process after "pruning" the master process with
`prune_bundler`. This is useful for activating gems that need to be
loaded in the master process, such as `puma_worker_killer`.

The integration test for `extra_runtime_dependencies` tested the
`$LOAD_PATH` of the worker process instead. Since workers are forked
from the master, it's normally fine to do this, but we might as well
test the master process's `$LOAD_PATH` directly if we can.
@cjlarose
Add test to refute that nio4r is loaded into puma master process
915b950
@cjlarose
Remove nio4r from puma master $LOAD_PATH
76e0f11
@cjlarose
Remove list of gems to activate from puma-wild
a3b5613
@cjlarose
Update History.md
fa5161e
@cjlarose cjlarose mentioned this pull request Oct 14, 2020
Closed
@nateberkopec nateberkopec added bug waiting-for-review Waiting on review from anyone labels Oct 14, 2020
@MSP-Greg
Copy link
Member

LGTM.

This code would normally not run on Windows (no fork, but maybe Windows 15?). There are a few places where ":" could be replaced by File::PATH_SEPARATOR. Thanks for the work.

@nateberkopec
Copy link
Member

Simple solution!

@nateberkopec
Copy link
Member

@cjlarose @MSP-Greg Do we still has this same issue w/ json?

@nateberkopec nateberkopec merged commit 6502c5b into puma:master Oct 20, 2020
@cjlarose
Copy link
Member Author

cjlarose commented Oct 20, 2020 •
edited

We still need to defer loading of the json gem to when the worker process loads (#2269). With that change, we're good: users can freely update the json gem in their applications and issue a phased restart. This PR basically does the same thing but for the nio4r gem. I'll add a test to ensure that the json gem isn't on the master processes' $LOAD_PATH to avoid a regression similar to #1801.

Edit: actually, I'm not 100% sure on the json gem. @MSP-Greg is the better resource on that.

Edit again: Opened #2436 to add an integration test to prevent regressions related to loading the json gem.

S3od22
S3od22 reviewed Oct 25, 2020
View reviewed changes
Copy link

@S3od22 S3od22 left a comment

Choose a reason for hiding this comment

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

Sss

@dentarg dentarg mentioned this pull request Oct 29, 2020
Closed
This was referenced Oct 29, 2020
Closed
Closed
@cjlarose cjlarose mentioned this pull request Oct 31, 2020
Merged
8 tasks
@cjlarose cjlarose mentioned this pull request Dec 17, 2020
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@S3od22 S3od22 S3od22 left review comments

Assignees
No one assigned
Labels
bug hacktoberfest-accepted waiting-for-review Waiting on review from anyone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

phased-restart: Could not find nio4r-2.5.1 in any of the sources (Bundler::GemNotFound)
4 participants
@cjlarose @MSP-Greg @nateberkopec @S3od22