ruby-i18n / i18n

Internationalization (i18n) library for Ruby
MIT License
986 stars 411 forks source link

[BUG] Simple backend not thread safe #554

Closed bjfish closed 3 years ago

bjfish commented 3 years ago

What I tried to do

I tried to run rails tests in parallel using threads and got test failures similar to the following example. https://github.com/rails/rails/blob/291a3d2ef29a3842d1156ada7526f4ee60dd2b59/activesupport/lib/active_support/test_case.rb#L83

Example Code

# frozen_string_literal: true

require 'bundler/inline'
gemfile(true) do
  source 'https://rubygems.org'
  gem 'i18n'
end

def run_test
    I18n.backend.store_translations(:en,
        translations: {
        templates: {
            found: { foo: "Foo" },
            found_yield_single_argument: { foo: "Foo" },
            found_yield_block: { foo: "Foo" },
            array: { foo: { bar: "Foo Bar" } },
            default: { foo: "Foo" },
            partial: { foo: "Partial foo" }
        },
        foo: "Foo",
        hello: "<a>Hello World</a>",
        html: "<a>Hello World</a>",
        hello_html: "<a>Hello World</a>",
        interpolated_html: "<a>Hello %{word}</a>",
        array_html: %w(foo bar),
        array: %w(foo bar),
        count_html: {
            one: "<a>One %{count}</a>",
            other: "<a>Other %{count}</a>"
        }
        }
    )

    I18n.translate(:'translations.missing', default: [[]])

    I18n.backend.reload!
end

I18n.load_path << Dir[File.expand_path("config/locales") + "/*.yml"]
threads = []
run_test()
1000.times do |i|
    threads << Thread.new do 
        sleep(rand(0))
        run_test()
   end
end
threads.map(&:join)

puts "Done"

What I expected to happen

prints: Done

What actually happened

#<Thread:0x00007feafdfa30a8 i18n.rb:43 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
    14: from i18n.rb:45:in `block (2 levels) in <main>'
    13: from i18n.rb:34:in `run_test'
    12: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n.rb:202:in `translate'
    11: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n.rb:202:in `catch'
    10: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n.rb:206:in `block in translate'
     9: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:32:in `translate'
     8: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/simple.rb:90:in `lookup'
     7: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/simple.rb:80:in `init_translations'
     6: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:18:in `load_translations'
     5: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:18:in `each'
     4: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:18:in `block in load_translations'
     3: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:230:in `load_file'
     2: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:230:in `each'
     1: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:230:in `block in load_file'
/Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/simple.rb:45:in `store_translations': undefined method `deep_merge!' for nil:NilClass (NoMethodError)
Traceback (most recent call last):
    14: from i18n.rb:45:in `block (2 levels) in <main>'
    13: from i18n.rb:34:in `run_test'
    12: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n.rb:202:in `translate'
    11: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n.rb:202:in `catch'
    10: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n.rb:206:in `block in translate'
     9: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:32:in `translate'
     8: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/simple.rb:90:in `lookup'
     7: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/simple.rb:80:in `init_translations'
     6: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:18:in `load_translations'
     5: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:18:in `each'
     4: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:18:in `block in load_translations'
     3: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:230:in `load_file'
     2: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:230:in `each'
     1: from /Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/base.rb:230:in `block in load_file'
/Users/bfish/.rbenv/versions/2.7.2/lib/ruby/gems/2.7.0/gems/i18n-1.8.7/lib/i18n/backend/simple.rb:45:in `store_translations': undefined method `deep_merge!' for nil:NilClass (NoMethodError)

Comments

Potentially unsafe code here: https://github.com/ruby-i18n/i18n/blob/v1.8.7/lib/i18n/backend/simple.rb#L43-L45

Maybe this should be a Concurrent::Hash?

radar commented 3 years ago

Thank you for the issue report. I can report that I can reproduce the issue with the steps you've provided.

Replacing the {} with Concurrent::Hash.new would be part of the solution, I think. The issue then is what to do as a substitute for deep_merge!, which is not a method on Concurrent::Hash objects.

radar commented 3 years ago

Ok, looks like deep_merge! will work. The tests seem happy with that commit.

Does that work for your case too, @bjfish?

bjfish commented 3 years ago

@radar Yes, this fixes the issue, thanks.

radar commented 3 years ago

@bjfish Great. I'll do a 1.8.8 release shortly with this commit included.

radar commented 3 years ago

All done. Thank you @bjfish.