Make the state hash fully thread safe #77
Conversation
|
This isn't enough, as the innermost data structure there is an array and should be something like a There are significant other thread safety concerns through the file that may need attention. Like if ev_for_fingerprint.empty?
ev_by_fingerprint.delete(event.fingerprint)
endIn general, using thread-safe data structures is not enough to make a program thread-safe. However, the pull request is still an improvement. |
That what I was aiming for. Not to fix everything but to start somewhere. |
There was a problem hiding this comment.
I appreciate you making this PR and bringing this issue to my attention. I want to release a new version shortly and would like to have these changes be part of it. Would you have time to address my comments? It'd be good to include an entry in the changelog as well. I record any internal changes that are considered fixes and this seems to fall into this category. The note can say, for example, Fix a concurrency issue in callbacks initialization or similar.
lib/finite_machine/hooks.rb
Outdated
| @@ -18,7 +18,7 @@ class Hooks | |||
| def initialize | |||
| @hooks_map = Concurrent::Map.new do |events_hash, hook_event| | |||
| events_hash[hook_event] = Concurrent::Map.new do |state_hash, name| | |||
There was a problem hiding this comment.
By the same token, shouldn't the events_hash be changed as well?
lib/finite_machine/hooks.rb
Outdated
| @@ -18,7 +18,7 @@ class Hooks | |||
| def initialize | |||
| @hooks_map = Concurrent::Map.new do |events_hash, hook_event| | |||
| events_hash[hook_event] = Concurrent::Map.new do |state_hash, name| | |||
| state_hash[name] = [] | |||
| state_hash.compute_if_absent(name) { [] } | |||
There was a problem hiding this comment.
If we're making things safer, it would probably make sense to convert this array to Concurrent::Array.new?
|
I'm sorry @piotrmurach but I ATM do not have capacity to apply any fixes to this code anymore. I can close this PR if you are ok with this as I won't be able to do it :( |
|
@mensfeld Thank you for the swift reply. I wanted to check with you first and give you a chance to claim this contribution. I'm keen to fix this one way or another. It's up to you what you feel is most appropriate. You can leave it open and I'll force push commits and then create a multi-author commit and merge it in. Or you can close this PR and I'll make a separate commit that fixes all the noted issues and take all the glory 👿 . I'd like you to be mentioned since you kindly brought this issue up. This would also leave a nice trace as to why things were done. No work on your part would be needed. |
Go for it! Once again sorry I cannot help but at the moment I am overloaded with my own OSS and it's hard for me to context switch |
Fixes issue described here: ruby-concurrency/concurrent-ruby#970
|
Thank you :) |
Fixes issue described here: ruby-concurrency/concurrent-ruby#970
Describe the change
Makes the state hash thread-safe
Why are we doing this?
Because the author (you) may not be aware about a potential race condition here.
Benefits
One less thing to worry about in case ppl use it heavily from the require moment in multiple threads.
Drawbacks
Hard to debug issues. Took me weeks to stop it in Karafka.
Requirements
masterbranch?