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

[Guides] Bug in "Managing the Locale across Requests" section with Puma #34043

Closed
tegon opened this Issue Oct 1, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@tegon

tegon commented Oct 1, 2018

Context

I was reviewing a colleague's pull request when I saw the following code:

before_action :set_locale
 
def set_locale
  I18n.locale = params[:locale] || I18n.default_locale
end

I commented that since we use Puma, this could cause issues because we're changing a global value in a not thread-safe way. What might happen is that while one request changes the locale, another using I18n.locale directly might get the changed locale instead of the default one - which can be assumed as the value we'll get when we do not change the locale in a request.
I know that because I've seen this I18n issue before.
An approach that would work is to use I18n.with_locale:

around_action :switch_locale

def switch_locale(&action)
  locale = params[:locale] || I18n.default_locale

  I18n.with_locale(locale, &action)
end

He answered that he got this solution straight from the Rails guides, in the following section: https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests.

This might look obvious - a global value is changed, so it will affect other requests too - but it might not be clear to everyone, especially beginners. And since Puma is a very popular choice these days to work with Rails, I decided to open this issue to see if you're willing to accept a patch to change the guides.

Sample repository

I created a sample repository where this can easily be tested. The relevant files are https://github.com/tegon/rails-guides-locales-bug/blob/master/config/environments/development.rb#L62, https://github.com/tegon/rails-guides-locales-bug/blob/master/app/controllers/pages_controller.rb and
https://github.com/tegon/rails-guides-locales-bug/blob/master/app/controllers/locales_controller.rb

If you just want to see it in action, there are some gifs below too:

I18n.locale=

i18-global-puma

I18n.with_locale

i18-with-locale-puma

Steps to reproduce

  • Clone the repository: https://github.com/tegon/rails-guides-locales-bug
  • Run bin/setup
  • Rails bin/rails server
  • Fire multiple requests that changes I18n.locale: for i in seq 100; do; curl http://localhost:3000/home\?locale\=pt-BR; echo ""; done
  • Fire multiple requests to show the value of I18n.locale: for i in seq 100; do; curl http://localhost:3000/locale ; echo ""; done
  • Check whether the values of the second requests show some pt-BR between the en values

System configuration

Rails version: 5.2.1

Ruby version: 2.5.0

Proposed solution

The way I see it we could either:

  • Change the docs to use the I18n.with_locale with an around_action
  • Keep the current example and add notes to warn that it might leak the local in multi-threaded environments, alongside the I18n.with_locale example.

Please let me know your thoughts on it. I will be happy to send a pull request for this if you agree with this. Thanks!

@y-yagi y-yagi added the docs label Oct 1, 2018

@deivid-rodriguez deivid-rodriguez referenced this issue Oct 5, 2018

Merged

Thread-safe locale switching #4237

0 of 5 tasks complete

leio10 added a commit to podemos-info/census that referenced this issue Oct 17, 2018

leio10 added a commit to podemos-info/census that referenced this issue Oct 17, 2018

Better locale handling (#219)
* Set locale on all pages

* Thread-safe locale switching

See rails/rails#34043

@gmcgibbon gmcgibbon added the i18n label Oct 29, 2018

@gmcgibbon

This comment has been minimized.

Member

gmcgibbon commented Oct 29, 2018

I think changing the docs would be the best solution. Please feel free to submit a PR to fix this. Thanks!

@gmcgibbon

This comment has been minimized.

Member

gmcgibbon commented Oct 30, 2018

I've submitted a PR for to amend the docs. You've done a lot of detective work, so I've given you credit as well for the contributors app to pick up on.

@lsylvester

This comment has been minimized.

Contributor

lsylvester commented Oct 31, 2018

I18n.locale is setting the value within the current thread - so it is threadsafe, and I18n.with_locale is using it internally.

The only thing that is being changed in behaviour is that subsequent requests on the same thread won't have the locale from the previous request set before the before/around_action is called - but that is an issue that could also happen on process based app servers - so it isn't really Thread safe related.

This issue in svenfuchs/i18n#381 (which this would resolve) is more related to thread/process pools if the I18n.locale is not reliably set on every request.

I think that we could consider promote using ActiveSupport::CurrentAttributes#resets to clear the I18n.locale after each request.

@gmcgibbon

This comment has been minimized.

Member

gmcgibbon commented Oct 31, 2018

🤦‍♂️ Thanks @lsylvester, I probably should've checked the method definition. I think we can all still agree this should be mentioned in the docs in some way. I'll adjust the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment