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] Simple backend not thread safe #554

Closed
bjfish opened this issue Jan 27, 2021 · 5 comments
Closed

[BUG] Simple backend not thread safe #554

bjfish opened this issue Jan 27, 2021 · 5 comments

Comments

@bjfish
Copy link
Contributor

bjfish commented Jan 27, 2021

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
Copy link
Collaborator

radar commented Feb 1, 2021

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 added a commit that referenced this issue Feb 1, 2021
@radar
Copy link
Collaborator

radar commented Feb 1, 2021

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

Does that work for your case too, @bjfish?

@bjfish
Copy link
Contributor Author

bjfish commented Feb 1, 2021

@radar Yes, this fixes the issue, thanks.

@radar
Copy link
Collaborator

radar commented Feb 2, 2021

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

@radar
Copy link
Collaborator

radar commented Feb 2, 2021

All done. Thank you @bjfish.

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

No branches or pull requests

2 participants