Thread Safety Fix #352
Thread Safety Fix #352
Conversation
Replace them with Concurrent::Hash
|
@radar, Re the PR: you should be using What about all other places that I swapped |
lib/i18n.rb
Outdated
| @@ -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 } | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@romuloceccon Could you please submit a PR to my branch here to fix that?
There was a problem hiding this comment.
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.
The idiom for lazy initialization with operator `||=` is not thread safe. Method `I18n#normalized_key_cache` was converted into a class variable initialized during module load to avoid a race condition.
…rovement Initialize global hash during module load
i18n.gemspec
Outdated
| @@ -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' | |||
There was a problem hiding this comment.
could we, eventually, not have the version locked down so specifically but instead e.g. ~> 1.0
|
ah I see, this is simply stale for almost a year, what's the hold up anything this needs help with? |
* master: (45 commits) Add regression test for #378 Bump to 0.8.6 Add fallback_in_progress to RESERVED_KEYS list Bump to 0.8.5 Fixes #369 thread issue when calling translate with fallbacks Remove gemfiles/Gemfile.*.lock from the repo Improve error message for missing pluralization key Bump to 0.8.4 Revert "Don't allow nil to be submitted as a key to i18n.translate()" Bump to 0.8.3 Update Changelog Handle false as a key correctly Bump Gemfiles Bump to 0.8.2 Add Gemfile.lock for each Rails version Bump to 0.8.1 Fix transliteration to default replacement char Docs: Add 0.8.0 to changelog No need to skip ruby 2.4+ x rails 4 now CI against newest stable rubies for each minor version ...
…nto pr-51-thread-safety-fix * 'pr-51-thread-safety-fix' of github.com:svenfuchs/i18n: Initialize global hash during module load
|
@kares I assume you're interested in this because you have a problem that would be fixed by either this PR or one very close to it. So based on that assumption, I'm going to ask you if you'd be available to test this PR to see if it would solve your issues. Please do try it. Thanks! |
radar
force-pushed
the
pr-51-thread-safety-fix
branch
from
7858213
to
3da644f
Compare
|
Thanks Ryan, I expected some but than for now it turned out to be something else. |
|
With #384 done, I think this can be merged now. |
This is an alternative take on #51, using concurrent-ruby instead of the thread_safe gem.