soveran / ohm

Object-Hash Mapping for Redis
http://ohm.keyvalue.org
MIT License
1.4k stars 167 forks source link

Update ohm.rb #242

Closed ioquatix closed 1 year ago

ioquatix commented 4 years ago

Don't lazy initialize mutex, it's not thread safe.

If you intended to have one unique mutex per class (e.g. derived classes have a unique mutex) I believe something like this is required...

class Model
    @mutex = Mutex.new

    def self.inherited(derived)
        derived.instance_variable_set(:@mutex, Mutex.new)
    end

    def self.mutex
        @mutex
    end

    def self.synchronize(&block)
        mutex.synchronize(&block)
    end
end

class MyModel < Model
end

# These are both different:
pp Model.mutex
pp MyModel.mutex
soveran commented 4 years ago

I've tried the patch and I get errors when running the tests. Do you think it has to be an instance variable @mutex or it can be a class variable @@mutex?

ioquatix commented 4 years ago

Do you have CI? e.g. GitHub Actions.

You could experiment with the alternative code I gave you which makes it per-class rather than shared.

soveran commented 4 years ago

I tried it! But I get an error. You don't get errors?

ioquatix commented 4 years ago

I tested the code in isolation to check it behaved the same, but not running tests for this project. Can you set up CI?

soveran commented 4 years ago

Yes, I'll do it tomorrow as it's already late here. Thanks!

ioquatix commented 1 year ago

Bump :)