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

Remove unnecessary isolation_level setting from load_defaults 7.0 #46617

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

r7kamura
Copy link
Contributor

Motivation / Background

The default value of ActiveSupport::IsolatedExecutionState.isolation_level is set to :thread, regardless of whether load_defaults 7.0 is used or not.

If so, there is no need to set this in load_defaults 7.0. This Pull Request has been created to remove this unnecessary setting.

Or if there is any reason to set this in load_defaults 7.0, I would like to know that.

Detail

This Pull Request simply removes this setting.

Additional information

I confirmed that ActiveSupport::IsolatedExecutionState.isolation_level returns :thread on Rails 7.0.4 with load_defaults 6.1.

$ bin/rails c -e test
Loading test environment (Rails 7.0.4)
irb: warn: can't alias context from irb_context.
irb(main):001:0> ActiveSupport::IsolatedExecutionState.isolation_level
=> :thread
irb(main):002:0> exit

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.
  • 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.
  • CI is passing.

@rails-bot rails-bot bot added the railties label Nov 30, 2022
@skipkayhil
Copy link
Member

If this is removed we should clean up the docs as well:

- [`config.active_support.isolation_level`](#config-active-support-isolation-level): `:thread`

and if backported to 7-0-stable it should probably be removed from new_framework_defaults_7_0:

# Define the isolation level of most of Rails internal state.
# If you use a fiber based server or job processor, you should set it to `:fiber`.
# Otherwise the default of `:thread` if preferable.
# Rails.application.config.active_support.isolation_level = :thread

@rails-bot rails-bot bot added the docs label Nov 30, 2022
@r7kamura
Copy link
Contributor Author

Good point. I agree that and I will make that change in this Pull Request, at least for guides/source/configuring.md.

@skipkayhil
Copy link
Member

@r7kamura Would you mind rebasing? I'll see if I can get it a review

The default value of `ActiveSupport::IsolatedExecutionState.isolation_level` is set to `:thread`, regardless of whether `load_defaults 7.0` is used or not.

- https://github.com/rails/rails/blob/v7.0.4/activesupport/lib/active_support/isolated_execution_state.rb

If so, there is no need to set this in `load_defaults 7.0`. This Pull Request has been created to remove this unnecessary setting
@r7kamura r7kamura force-pushed the feature/isolation-level-default branch from e5e37be to 7f52979 Compare August 9, 2023 05:40
@rafaelfranca rafaelfranca merged commit 008db52 into rails:main Aug 9, 2023
9 checks passed
rafaelfranca added a commit that referenced this pull request Aug 9, 2023
Remove unnecessary isolation_level setting from load_defaults 7.0
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

3 participants