Repository navigation
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.
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.
@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.
radar
left a comment
radar
left a comment
There was a problem hiding this comment.
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::MiddlewareI 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
localeis 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