-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
Introduce middleware that resets current local thread's config after each request #382
Conversation
…each request This is a proposal PR. Rack based apps that work in a multithreaded environment can plugin this middleware to make sure a context of `locale` is only persisted in the lifecycle of the request and aren't leaked between requests unintentionally. Which is common in thread pool like environments. This is especially helpful if a server/environment isn't cleaning thread local storage after/before request.
def call(env) | ||
@app.call(env) | ||
ensure | ||
Thread.current[:i18n_config] = I18n::Config.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this will cause the translations to be reloaded upon every single request. For applications with large amounts of translations, this can lead to an unnecessary slow down on every request.
Since your issue seems to be just that the default locale is not used again for I18n.locale
, why not have here instead:
I18n.locale = I18n.default_locale
?
I think this would accomplish what you wish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radar thanks for the review, I don't think it will cause the translations to be re-loaded, as Backend::Simple
is initialized as a class variable and once rails is initialized, the same object is persisted/re-used:
You can test this by going to rails console
and checking that backend
's object_id
is always the same:
2.3.3 :003 > I18n::Config.new.backend.object_id
=> 70171618106860
2.3.3 :004 > I18n::Config.new.backend.object_id
=> 70171618106860
That said, since the main issue is of TLS in a multi-threaded environemt, updating Thread.current
itself made sense :). Lmk, if I missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think re-setting I18n.locale
in the middleware would be a better approach than re-initializing the configuration every single time.
I believe your method of re-init I18n::Config.new
will reinitialize everything, including the backend used by I18n
. This will then re-trigger a I18n::Backend::Base#load_translations
call, which can take a long time if there are a lot of files / translations available for the app.
I think setting locale in the middleware is a better approach.
What do you think?
@radar lmk if what I mentioned appears to be incorrect in any way or if I am missing something :) |
LGTM @shayonj. Thanks for your work on this! |
With all the issues now complete in 0.9.0, I will look at doing a release this week.
… On 13 Oct 2017, at 03:03, Shayon Mukherjee ***@***.***> wrote:
@radar lmk if what I mentioned appears to be incorrect in any way or if I am missing something :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I'm a bit late to this party, but just wanted to confirm the default behavior after a year. So, in order to make the current locale value thread-safe, we need to include the middleware on Rack applications like this? config.middleware.use ::I18n::Middleware I see the middleware is still on master so I guess that's correct. Any info would be awesome @radar @shayonj |
@mcelicalderon this is correct |
This is a proposal PR.
Rack based apps that work in a multithreaded environment can plugin this
middleware to make sure a context of
locale
is only persistedin the lifecycle of the request and aren't leaked between requests unintentionally.
Which is common in thread pool like environments.
This is especially helpful if a server/environment isn't cleaning thread local
storage after/before request.
Issue: #381
Update: If this PR is merged, users will need to explicitly require the new middleware
Example:
config/application.rb