Remove threadsafe gem dependency #45

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@mperham
Contributor

mperham commented Feb 3, 2016

This PR includes the whitespace PR I just sent. If you merge that PR, you should just see the relevant changes here.

This change removes the thread_safe gem dependency and instead uses a Mutex as a write barrier to ensure multiple threads can't write to the Hash at the same time.

mperham added some commits Feb 3, 2016

Remove thread_safe gem
Since the countries and timezones hash are mostly read only, we can wrap the only write operation in a Mutex.
@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Feb 3, 2016

(I believe) it's unsafe to read a hash at the same time it's being written -- it's not just writes that need the mutex.

cc @thedarkone

matthewd commented Feb 3, 2016

(I believe) it's unsafe to read a hash at the same time it's being written -- it's not just writes that need the mutex.

cc @thedarkone

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Feb 3, 2016

Member

Thanks for submitting the PRs. I hadn't spotted that the thread_safe gem is being deprecated.

Is synchronizing just Hash write operations guaranteed to be safe on all Ruby implementations? ThreadSafe::Cache and the replacement Concurrent::Map use this approach for MRI, but have alternative implementations for JRuby, Rubinius, etc.

Member

philr commented Feb 3, 2016

Thanks for submitting the PRs. I hadn't spotted that the thread_safe gem is being deprecated.

Is synchronizing just Hash write operations guaranteed to be safe on all Ruby implementations? ThreadSafe::Cache and the replacement Concurrent::Map use this approach for MRI, but have alternative implementations for JRuby, Rubinius, etc.

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Feb 3, 2016

Contributor

I wasn't sure if reading was an issue. The alternative is to pull in concurrent-ruby instead of thread_safe.

Contributor

mperham commented Feb 3, 2016

I wasn't sure if reading was an issue. The alternative is to pull in concurrent-ruby instead of thread_safe.

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Feb 4, 2016

Member

I'm happy with switching from thread_safe to concurrent-ruby.

tzinfo currently has a required_ruby_version of >= 1.8.7. This would need changing to >= 1.9.3 to match concurrent-ruby. Some of the code to support Ruby 1.8 in and around ruby_core_support.rb could also then be dropped.

Member

philr commented Feb 4, 2016

I'm happy with switching from thread_safe to concurrent-ruby.

tzinfo currently has a required_ruby_version of >= 1.8.7. This would need changing to >= 1.9.3 to match concurrent-ruby. Some of the code to support Ruby 1.8 in and around ruby_core_support.rb could also then be dropped.

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Feb 4, 2016

Contributor

@philr Yeah, you just need to move from ThreadSafe::Hash to Concurrent::Hash. Exact same semantics.

Contributor

mperham commented Feb 4, 2016

@philr Yeah, you just need to move from ThreadSafe::Hash to Concurrent::Hash. Exact same semantics.

@mperham

This comment has been minimized.

Show comment
Hide comment
@mperham

mperham Feb 4, 2016

Contributor

Feel free to close this and do that instead.

Contributor

mperham commented Feb 4, 2016

Feel free to close this and do that instead.

@thedarkone

This comment has been minimized.

Show comment
Hide comment
@thedarkone

thedarkone Feb 4, 2016

@matthewd is right, technically it is not enough to have synchronization just on the write side. I will readily admit that right now, on all the current Ruby VMs, when run on x86 hardware, synchronization on the read side is not required, but I'm pretty sure that this is going to change on the new gen of runtimes (namely truffle+jruby).

@philr if swapping thread_safe for concurrent-ruby (concurrent-ruby is a larger code base) is not an issue, go for it. Concurrent::Map is a carbon copy of ThreadSafe::Cache.

PS: @matthewd, thx for the ping.

@matthewd is right, technically it is not enough to have synchronization just on the write side. I will readily admit that right now, on all the current Ruby VMs, when run on x86 hardware, synchronization on the read side is not required, but I'm pretty sure that this is going to change on the new gen of runtimes (namely truffle+jruby).

@philr if swapping thread_safe for concurrent-ruby (concurrent-ruby is a larger code base) is not an issue, go for it. Concurrent::Map is a carbon copy of ThreadSafe::Cache.

PS: @matthewd, thx for the ping.

@philr

This comment has been minimized.

Show comment
Hide comment
@philr

philr Feb 4, 2016

Member

@thedarkone thanks for the confirmation regarding write-only synchronization.

The larger code base of concurrent-ruby is unlikely to be an issue. I think the majority of TZInfo users are using it through ActiveSupport and ActiveSupport has already switched to concurrent-ruby for the upcoming Rails 5 release.

I've raised #46 for switching to Concurrent::Map.

Member

philr commented Feb 4, 2016

@thedarkone thanks for the confirmation regarding write-only synchronization.

The larger code base of concurrent-ruby is unlikely to be an issue. I think the majority of TZInfo users are using it through ActiveSupport and ActiveSupport has already switched to concurrent-ruby for the upcoming Rails 5 release.

I've raised #46 for switching to Concurrent::Map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment