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

Don't swallow exception in actionmailer/actionmailbox when requiring "mail" fails #46898

Conversation

chitty
Copy link

@chitty chitty commented Jan 5, 2023

Motivation / Background

PR #44600 introduced a fallback for missing system dependencies related to the Mail gem. That PR had the unintended side effect of swallowing exceptions when require "mail" fails.

For example, recently, there was a regression in the Mail gem, where some of the files had their permission removed. This caused issues during the Rails initialization process when certain conditions were met:

  • rails 6.1.x
  • ruby 3.0.x
  • mail 2.8.0
  • config.eager_load = true
  • gems installed using a different system user from the one used by http server process (e.g.: most docker-based installations)

require "mail" under these conditions would fail but Rails would swallow the exception, causing eager_load (and potentially other calls) to fail with weird error messages.

Vanilla rails app with minimal settings changes to demonstrate the bug: https://github.com/chitty/repro-46898

Detail

This PR changes the code introduced in #44600 so that it always re-raises the exception.

This does not affect Rails 7 since the fallback isn't needed in that case, as mentioned in #44600:

For Rails 7 we could just make those gems dependencies of the framework.
But in Rails 6.1, which we need to support old rubies, installing those
gems can cause trouble. So instead we are intercepting the error and
telling people to add the gem to the Gemfile.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature. (Didn't add tests, but added a link to a repo that reproduces the bug being solved)
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actioncable label Jan 5, 2023
@chitty chitty changed the base branch from main to 6-1-stable January 5, 2023 16:04
This will raise the proper error to the user for easier debugging instead of swallowing the error when the conditional is not true
@chitty chitty force-pushed the chitty--do-not-swallow-exception-in-action_mailer branch from f8c5118 to 2849716 Compare January 5, 2023 22:08
@chitty chitty changed the title Don't swallow exception in action mailer when requiring "mail" fails Don't swallow exception in actionmailer/actionmailbox when requiring "mail" fails Jan 5, 2023
@byroot
Copy link
Member

byroot commented Jan 6, 2023

Hum. 6.1 CI is in very bad shape (in good part because of the mail gem), but the change makes sense to me.

@byroot byroot merged commit 238b632 into rails:6-1-stable Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants