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

Introduce middleware that resets current local thread's config after each request #382

Merged
merged 1 commit into from Oct 14, 2017

Conversation

@shayonj
Copy link
Contributor

@shayonj shayonj commented Aug 27, 2017

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.

Issue: #381


Update: If this PR is merged, users will need to explicitly require the new middleware

Example:

config/application.rb

config.middleware.use ::I18n::Middleware
…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

This comment has been minimized.

@radar

radar Oct 1, 2017
Collaborator

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.

This comment has been minimized.

@shayonj

shayonj Oct 1, 2017
Author Contributor

@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:

https://github.com/svenfuchs/i18n/blob/6ab51c78be833eb011d5b4d3f5b2c54e74847512/lib/i18n/config.rb#L19

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.

Copy link
Collaborator

@radar radar left a 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?

@shayonj
Copy link
Contributor Author

@shayonj shayonj commented Oct 12, 2017

@radar lmk if what I mentioned appears to be incorrect in any way or if I am missing something :)

@radar
Copy link
Collaborator

@radar radar commented Oct 14, 2017

LGTM @shayonj. Thanks for your work on this!

@radar radar merged commit efc97ba into ruby-i18n:master Oct 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radar
Copy link
Collaborator

@radar radar commented Oct 14, 2017

@mcelicalderon
Copy link

@mcelicalderon mcelicalderon commented May 8, 2019

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

@radar
Copy link
Collaborator

@radar radar commented May 8, 2019

@mcelicalderon this is correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants