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

I18n::Backend::Memoize is not thread-safe #51

Closed
wants to merge 1 commit into from

Conversation

thedarkone
Copy link
Contributor

Backend::Memoize uses a plain Hash that is shared between all threads, this is obviously not thread-safe and the module can't be used in truly threaded environments such as JRuby.

@josevalim
Copy link
Contributor

Is there an easy way for us to solve this which does not require wrapping up each interaction in a mutex?

@thedarkone
Copy link
Contributor Author

None that I know of :(. We'd need Ruby version specific "memoize-friendly" hash implementation (ie. you're fine with plain Hash on MRI/YARV, use some kind of thin wrapper around ConcurrentHashMap on JRuby, something else for future GIL-free Rubinius etc).

Using a global hash on JRuby is basically like playing russian roulette, most of the time you'll probably be fine, however if you are unlucky you might run into a random java.lang.ArrayIndexOutOfBoundsException (survivable) or you'll get with X threads stuck in a infinite loop (kinda sucks for production environment).

Try it for yourself: http://gist.github.com/559673

PS: btw we're in (good?) company with Rails 3 on this one .. https://rails.lighthouseapp.com/projects/8994/tickets/5340

@josevalim
Copy link
Contributor

So the issue here and in the Rails 3 ticket is that we need to use ConcurrentHashMap in JRuby? Do you think you can provide a small patch? Thanks a lot!

Replace them with ThreadSafe::Cache.
@thedarkone
Copy link
Contributor Author

Turning this into a PR.

@thedarkone
Copy link
Contributor Author

Note to self: I18n::Locale::Fallbacks is also not thread safe.

@radar
Copy link
Collaborator

radar commented Nov 20, 2016

@thedarkone Would you be interested in updating this to use something more modern, such as concurrent-ruby? Or whatever you choose, I guess.

@radar radar added this to the 0.9.0 milestone Nov 20, 2016
@radar
Copy link
Collaborator

radar commented Nov 20, 2016

Actually, I'll make an attempt at a concurrent-ruby version of this patch now.

@radar radar self-assigned this Nov 20, 2016
@radar radar mentioned this pull request Nov 20, 2016
@radar
Copy link
Collaborator

radar commented Nov 20, 2016

@thedarkone I'd like your feedback on #352 before I merge it, if you have time.

@radar radar modified the milestones: 0.9.0, 0.10.0 Oct 1, 2017
kares added a commit to kares/i18n that referenced this pull request Oct 1, 2017
... a sometimes reproducer  for ruby-i18n#51 (needs a GIL free Ruby such as JRuby)
@lostapathy
Copy link
Contributor

@thedarkone or @radar can this be closed now that #352 has been merged or are you looking for a PR for something else to complete it?

@radar
Copy link
Collaborator

radar commented Oct 13, 2017

Yes this can now be closed.

@radar radar closed this Oct 13, 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants