handler (storage) thread-safety #5
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ved concurrency) this is mostly a safe-bet ... without additional code the mixin is not fully thread-safe!
also use https source
sfgeorge
reviewed
Nov 16, 2016
| @@ -18,6 +18,8 @@ Gem::Specification.new do |s| | |||
| s.executables = `git ls-files -- bin/*`.split("\n").map{ |f| File.basename(f) } | |||
| s.require_paths = ["lib"] | |||
|
|
|||
| s.add_dependency 'thread_safe', ["~> 0.3"] | |||
There was a problem hiding this comment.
@kares Sorry for being very slow on my end. Question for you, should this be [">= 0.3.4"]?
There was a problem hiding this comment.
when I originally did this 0.3.x was latest - was probably afraid that it might broke around 1.0
but since thread_safe is dead and part of concurrent_ruby gem, latest (last) release is 0.3.5.
so this shouldn't matter much really - but I update to make sure < 0.3.4 is not used - thanks.
|
Thanks very much @kares. I think this looks great 👍 @benlangfeld could you take a look when you get a chance? |
benlangfeld
requested changes
Nov 18, 2016
benlangfeld
approved these changes
Nov 18, 2016
|
Awesome! Thank you both! |
kares
added a commit
to kares/has-guarded-handlers
that referenced
this pull request
Jan 14, 2019
* concurrent-ruby: [CI] let's test against more (recent) Rubies use concurrent/map as a thread_safe replacement ~~handler (storage) thread-safety (adhearsion#5)~~
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HGH's design is not thread-safe since most of the use-cases are actors themselves - thus protected from concurrent access. However, not all and esp. under JRuby (with non-GIL concurrency and likely due threaded fibers) some parts tend to break unexpectedly when a handler update is executed by 2 threads.
Here's a failure from the older stack which is still relevant today (with an updated Adhearsion 2.6 stack) :
@handlers ||= Hash.new { |h, k| h[k] = Hash.new { |h, k| h[k] = [] } }the underlying issue is that when the same key is "missed" (under 2 threads) the default proc might execute twice ... withTS::Cachethere's an atomic guarantee offetch_or_storemethod - once set we're only retrieve the same value for the given keyp.s.
ThreadSafe::Cacheis part of the concurrent-ruby gem these days, if there's interest in getting the changes upstream it should be straightforward to migrate usingConcurrent::Hashinstead.