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

[BUG] i18n translation loading is not thread-safe #643

Closed
mensfeld opened this issue Nov 20, 2022 · 3 comments · Fixed by #644
Closed

[BUG] i18n translation loading is not thread-safe #643

mensfeld opened this issue Nov 20, 2022 · 3 comments · Fixed by #644

Comments

@mensfeld
Copy link
Contributor

mensfeld commented Nov 20, 2022

What I tried to do

Try to load several languages in different threads:

# irb -r ./lib/i18n
100.times.map do
  Thread.new do
    I18n.backend.store_translations(rand.to_s, :foo => { :bar => 'bar', :baz => 'baz' })
  end
end.each(&:join)

Check how many languages are loaded:

I18n.available_locales.count #=> 1

Consecutive executions give correct result:

# run after initial count 1
100.times { Thread.new { I18n.backend.store_translations(rand.to_s, :foo => { :bar => 'bar', :baz => 'baz' }) } }
I18n.available_locales.count #=> 101

What I expected to happen

I expected all the languages to be loaded and not one.

What actually happened

This: https://github.com/ruby-i18n/i18n/blob/master/lib/i18n/backend/simple.rb#L71

@translations ||= Concurrent::Hash.new { |h, k| h[k] = Concurrent::Hash.new }

is not thread-safe. The hash initialization for a given language may not be set correctly because it will be evaluated several times.

Versions of i18n, rails, and anything else you think is necessary

i18n taken from master.

Other info

Happy to fix this with a PR. This may seem like a weird edge case though the side effects may also be weird: some translations may not be loaded as expected while others may work and it may be close to impossible to figure out the reason.

@nijikon
Copy link

nijikon commented Nov 20, 2022

@radar
Copy link
Collaborator

radar commented Nov 20, 2022

Yeah, that definitely seems like a bug. Could you please submit a PR to fix this up?

@mensfeld
Copy link
Contributor Author

@radar there are two ways to fix this and want to make sure I pick one that is more suitable:

  1. Replace Concurrent::Hash with Concurrent::Map including checks that their API coverage is the same for i18n.
  2. Wrap the initialization operation with a Mutex.

Any of this will work. I would prefer 2 because it brings less risk to the table and mutex will apply only for initial loading, not for the cached state.

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

Successfully merging a pull request may close this issue.

3 participants