Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The lock method conflicts with ActiveRecord::Querying#lock #196

Closed
UniIsland opened this issue Mar 14, 2016 · 6 comments
Closed

The lock method conflicts with ActiveRecord::Querying#lock #196

UniIsland opened this issue Mar 14, 2016 · 6 comments

Comments

@UniIsland
Copy link

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 .

@nateware
Copy link
Owner

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

@khiav223577
Copy link

khiav223577 commented Mar 9, 2018

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

@nateware
Copy link
Owner

CC @tmsrjs - another issue related to lock that may be worth looking at in version 2.0.0

@chbonser
Copy link

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

@nateware
Copy link
Owner

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

@nateware
Copy link
Owner

This has been fixed in 2.0.0.alpha and pushed to rubygems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants