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

Make i18n locale setting docs use around_action #34356

Merged
merged 1 commit into from Oct 31, 2018

Conversation

Projects
None yet
4 participants
@gmcgibbon
Member

gmcgibbon commented Oct 30, 2018

Summary

Makes examples in the I18n docs that switch locales in controllers thread-safe use around_action in order to be less susceptible to leakage.

Closes #34043.

@rails-bot rails-bot bot added the docs label Oct 30, 2018

@gmcgibbon

This comment has been minimized.

Member

gmcgibbon commented Oct 30, 2018

Show resolved Hide resolved guides/source/i18n.md Outdated
Make i18n locale setting docs use around_action
Changes `I18n.locale` assignment in docs to use `I18n.with_locale`
in `around_action` to ensure locale resetting after action processing.

[ci skip]

[Gannon McGibbon + Leonardo Tegon]

@gmcgibbon gmcgibbon changed the title from Make i18n locale setting docs threadsafe to Make i18n locale setting docs use around_action Oct 31, 2018

@rafaelfranca rafaelfranca merged commit f98901c into rails:master Oct 31, 2018

1 check passed

codeclimate All good!
Details
@fxn

This comment has been minimized.

Member

fxn commented Oct 31, 2018

For the archives: This patch is not really about thread-safety. The writer I18n.locale= is of course thread-safe.

Indeed, the whole edit is dubious to me. Requests set a locale with high priority. No matter if done in a before_action or in an around_action, the previous locale (before) or default locale (around) values don't matter because the locale is set to the correct value for that request anyway.

The documentation used before_action because it is the simplest of the two.

@gmcgibbon

This comment has been minimized.

Member

gmcgibbon commented Nov 1, 2018

@fxn Thanks for clarifying. Yes, the docs now recommend a slightly more convoluted way of setting locales but I would argue its still better practice to reset the locale once you're done processing the controller action. As described in the issue, depending on the web server setup, locales can leak across requests.

If I understand this whole problem correctly, an important thing to highlight is to make sure to set the locale on every request if you use before_action. Recommending the around_action approach makes the code more complex, but less susceptible to leakage.

@gmcgibbon gmcgibbon deleted the gmcgibbon:docs_i18n_with_locale_threadsafe branch Nov 1, 2018

@fxn

This comment has been minimized.

Member

fxn commented Nov 1, 2018

I am about to take a plane, but don’t see how the server setup may lead to leakage. The setter is thread-safe, different controllers run in different threads. QED.

From the PR description I believe the patch is motivated by a misunderstanding (and your colleague deserves being told their code was fine).

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 1, 2018

It is not related to server setup, it is related to the application setup. The before_filter approach only doesn't leak the i18n configuration if you add it to all the controllers. As soon you forget to add to one controller that controller will use the leaked value as i18n. The around_filter never leaks. So even that that guide says: add this to you ApplicationController, if you application has an engine, inside the engine the controllers will use the leaked value for i18n.locale.

@fxn

This comment has been minimized.

Member

fxn commented Nov 1, 2018

Two remarks:

The documentation has the filter in ApplicationController, so it should conventionally apply to all controllers.

If you have a bug and forget to set the user’s locale in a controller, you have a bug and using the previous one is as incorrect as using the default one.

@fxn

This comment has been minimized.

Member

fxn commented Nov 1, 2018

Hey, let me be clear. I don’t want to revert this patch. Don’t like this style but I respect the merge.

My point is that the whole idea was to fix a threading issue that does not exist. If the patch is left, it has to be clear the reasoning is a different one.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Nov 1, 2018

ApplicationController only set the filter on the application's controllers. Engines don't use the ApplicationController.

The second case I agree, it is a bug in the application.

But the leaky locale in the application reaching out to engines is a real problem and I think using around_filter is the best way to solve it.

This problem, also happens with the ActiveSupport::Current but we do solve that by adding automatic reset of state when the to_prepare is run on any controller. That could be an alternative solution for the leaky problem but it would require us to change I18n in Rails to clean that leaky state.

I totally agree this is not fixing a threading issue. This is fixing a leaky state issue reaching out to controller mounted in the application using engine. That is a different issue of the one linked to this PR, but I still think it is worth to fix since most applications don't have control over their engines.

@fxn

This comment has been minimized.

Member

fxn commented Nov 1, 2018

Fair enough Rafael.

This is optimizing for a very particular use case in which you have controllers out of your control, and that serve stuff not subject to locale, for which the default locale is fine.

I can see that argument and no prob leaving this change on that basis 👍🏼.

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