piotrmurach / finite_machine

A minimal finite state machine with a straightforward syntax.
https://piotrmurach.github.io/finite_machine/
MIT License
808 stars 38 forks source link

Make the state hash fully thread safe #77

Closed mensfeld closed 1 year ago

mensfeld commented 1 year ago

Fixes issue described here: https://github.com/ruby-concurrency/concurrent-ruby/issues/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

granthusbands commented 1 year ago

This isn't enough, as the innermost data structure there is an array and should be something like a Concurrent::Array

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)
end

In general, using thread-safe data structures is not enough to make a program thread-safe. However, the pull request is still an improvement.

mensfeld commented 1 year ago

In 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.

mensfeld commented 1 year ago

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 :(

piotrmurach commented 1 year ago

@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.

mensfeld commented 1 year ago

@piotrmurach

. You can leave it open and I'll force push commits and then create a multi-author commit and merge it in.

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

mensfeld commented 1 year ago

Thank you :)