nateware/redis-objects

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
end

I 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
end
class User < ActiveRecord::Base
  def your_counter
    CounterService.new(id).your_counter
  end
end

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