-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thread safety not fully fixed
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.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relaxed in 5480dd3.
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! |
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.