The `lock` method conflicts with ActiveRecord::Querying#lock
UniIsland opened this issue · 6 comments
Class method lock defined in Redis::Objects::Locks::ClassMethods conflicts with lock in ActiveRecord::Querying#lock, which makes it impossible to enable ActiveRecord Pessimistic Locking by Account.lock.find(1).
We can still chain the method like this Account.wherer(id:1).lock.take. but that is dirty and better be avoided.
This issue could be part of #153 .
This is a tough one, because the design philosophy of Redis::Objects is that database locking is a terrible idea. On the other hand, I can see the argument that there could be edge cases where you would want to use ActiveRecord locking. I don't have a strong opinion other than I'm worried at this point that it would break backwards compatibility. I would be ok with changing this to something like rlock or redis_lock if we bumped the version to 2.0
It'll also break with_lock method if you include Redis::Objects in an ActiveRecord
EX:
class User < ActiveRecord::Base
include Redis::Objects
counter :your_counter
endI solved the problem by moving it to a service object
class CounterService
include Redis::Objects
attr_reader :id # for redis
def initialize(id)
@id = id
end
counter :your_counter
endclass User < ActiveRecord::Base
def your_counter
CounterService.new(id).your_counter
end
endCC @tmsrjs - another issue related to lock that may be worth looking at in version 2.0.0
@nateware I think this issue needs a fresh look. In particular, the fact that this collision causes Active Record's with_lock to silently fail to acquire a lock is super surprising and very hard to detect.
At a minimum, while this method name collision exists, I recommend updating the README to discourage mixing Redis::Objects into ActiveRecord models. @khiav223577's pattern plus adding method delegators is a fine workaround to recommend instead.
What is the consensus on this issues and the similar #191? I'm ok at this point if we want to rename the method to redis_lock or rlock (or similar) and bump the version to 2.0.0
This has been fixed in 2.0.0.alpha and pushed to rubygems