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

Thread Safety Fix #352

Merged
merged 8 commits into from Oct 10, 2017
Merged

Thread Safety Fix #352

merged 8 commits into from Oct 10, 2017

Conversation

@radar
Copy link
Collaborator

@radar radar commented Nov 20, 2016

This is an alternative take on #51, using concurrent-ruby instead of the thread_safe gem.

Replace them with Concurrent::Hash
@thedarkone
Copy link
Contributor

@thedarkone thedarkone commented Nov 20, 2016

@radar, ThreadSafe::Cache (with the rest of thread_safe) has been been merged into concurrent-ruby. It is called Concurrent::Map now.

Re the PR: you should be using Concurrent::Map instead of Concurrent::Hash (formerly ThreadSafe::Hash). Admittedly this is confusing, but as rule of thumb one should always pick Conc::Map over Conc::Hash unless insertion order is important.

What about all other places that I swapped Hash for TS::Cache in the original PR?

Copy link
Contributor

@romuloceccon romuloceccon left a comment

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 }

This comment has been minimized.

@romuloceccon

romuloceccon Mar 3, 2017
Contributor

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.

This comment has been minimized.

@radar

radar Mar 5, 2017
Author Collaborator

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

This comment has been minimized.

@romuloceccon

romuloceccon Mar 10, 2017
Contributor

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.

romuloceccon and others added 2 commits Mar 11, 2017
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
@@ -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'

This comment has been minimized.

@kares

kares Sep 29, 2017
Contributor

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

This comment has been minimized.

@radar

radar Oct 1, 2017
Author Collaborator

Relaxed in 5480dd3.

@kares
Copy link
Contributor

@kares kares commented Sep 29, 2017

ah I see, this is simply stale for almost a year, what's the hold up anything this needs help with?

radar added 3 commits Oct 1, 2017
* 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
@radar
Copy link
Collaborator Author

@radar radar commented Oct 1, 2017

@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 radar force-pushed the pr-51-thread-safety-fix branch to 3da644f Oct 1, 2017
@radar radar added this to the 0.9.0 milestone Oct 1, 2017
@radar
Copy link
Collaborator Author

@radar radar commented Oct 1, 2017

I'm targeting this and #382 for a 0.9 release, probably in the next week or two, assuming that both get tested / approved by then. So it would be really appreciated if you could test this, @kares.

@kares
Copy link
Contributor

@kares kares commented Oct 1, 2017

Thanks Ryan, I expected some but than for now it turned out to be something else.
Felt scary that this has been identified for so long and not handled right, I think we can come up with a threaded test-case to emulate a potential concurrent issue.

@radar
Copy link
Collaborator Author

@radar radar commented Oct 10, 2017

With #384 done, I think this can be merged now.

@radar radar merged commit bf58182 into master Oct 10, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@radar radar deleted the pr-51-thread-safety-fix branch Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants