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.

Sign up for GitHub

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

Jump to bottom

Thread Safety Fix #352

Merged
merged 8 commits into from Oct 10, 2017
Merged

Thread Safety Fix #352

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
77a565b
Avoid using global lazily populated Hashes.
radar Nov 20, 2016
a3a7d0d
Must require concurrent/hash for Concurrent::Hash to work
radar Nov 21, 2016
649d435
Initialize global hash during module load
romuloceccon Mar 11, 2017
9aabd00
Merge pull request #363 from romuloceccon/pr-51-thread-safety-fix-imp…
radar Mar 23, 2017
5480dd3
Relax concurrent-ruby version
radar Oct 1, 2017
358249b
Merge branch 'master' into pr-51-thread-safety-fix
radar Oct 1, 2017
6d2ec72
Merge branch 'pr-51-thread-safety-fix' of github.com:svenfuchs/i18n i…
radar Oct 1, 2017
3da644f
Switch to Concurrent::Map + use it in other places
radar Oct 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion Gemfile.lock
View file
Open in desktop
Expand Up @@ -2,10 +2,12 @@ PATH
remote: .
specs:
i18n (0.7.0)
concurrent-ruby (= 1.0.2)

GEM
remote: https://rubygems.org/
specs:
concurrent-ruby (1.0.2)
metaclass (0.0.4)
minitest (5.9.1)
mocha (1.2.1)
Expand All @@ -25,4 +27,4 @@ DEPENDENCIES
test_declarative

BUNDLED WITH
1.13.5
1.13.6
2 changes: 2 additions & 0 deletions i18n.gemspec
View file
Open in desktop
Expand Up @@ -19,4 +19,6 @@ Gem::Specification.new do |s|
s.rubyforge_project = '[none]'
s.required_rubygems_version = '>= 1.3.5'
s.required_ruby_version = '>= 1.9.3'

s.add_dependency 'concurrent-ruby', '1.0.2'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we, eventually, not have the version locked down so specifically but instead e.g. ~> 1.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relaxed in 5480dd3.

end
4 changes: 3 additions & 1 deletion lib/i18n.rb
View file
Open in desktop
@@ -1,3 +1,5 @@
require 'concurrent/hash'

require 'i18n/version'
require 'i18n/exceptions'
require 'i18n/interpolate/ruby'
Expand Down Expand Up @@ -335,7 +337,7 @@ def normalize_key(key, separator)
end

def normalized_key_cache
@normalized_key_cache ||= Hash.new { |h,k| h[k] = {} }
@normalized_key_cache ||= Concurrent::Hash.new { |h,k| h[k] = Concurrent::Hash.new }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While Concurrent::Hash solves the problem of accessing an already initialized normalized_key_cache from multiple threads, the code is still prone to a race condition before initialization. In such a situation two threads might create, each one, an instance of Concurrent::Hash (the outer one), but the loser will operate on an instance which will be later overwritten by the winner.

IMO @normalized_key_cache should be initialized during module load.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romuloceccon Could you please submit a PR to my branch here to fix that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, and sorry for the delay. I'll do it during the weekend. Meanwhile, here's a demonstration of why the code above is not thread safe: https://gist.github.com/romuloceccon/f44a30cb43f8081279a484193d386f55.

end
end

Expand Down