ionelmc / python-redis-lock

Lock context manager implemented via redis SET NX EX and BLPOP.
https://pypi.python.org/pypi/python-redis-lock
BSD 2-Clause "Simplified" License
550 stars 78 forks source link

Could not acquire or release lock when create lock with id field #44

Closed PeerXu closed 8 years ago

PeerXu commented 8 years ago

When I create a lock with id, the _held field forbid to acquire or release. _held should be set by id and redis lock:<name>, like:

def __init__(...):
  ...
  if id:
    if self.get_owner_id() == id:
      self._held = True
    else:
      raise AlreadyAcquired("Already acquired from this Lock instance")  # or else error?
  ...
ionelmc commented 8 years ago

Hmmm, indeed. Looks like we forgot to add that check. Are you interested in making a PR?

AndreiPashkin commented 8 years ago

Maybe just get rid of _held completely? Ownership checks are performed during each operation, like acquire or release, anyway.

PeerXu commented 8 years ago

In my opinion, _held is a helper for acquire and release operation like a cache. When acquire or release, save some redis request by checking _held.

AndreiPashkin commented 8 years ago

But there is no point in cache, because the whole point of using distributed lock is to not store a lock state locally.

ionelmc commented 8 years ago

Now that I look at this more I think we should discuss a bit before changing anything.

A bit of context: when I added the ability to override the id I had one and only one usecase in mind:

Allow releasing a lock from a different process.

Now, in this issue here, there's another usecase, maybe something like this?

Allow a fleet of processes (identified by the same id) to get the lock and release it.

@PeerXu, is that indeed your usecase? If not, please explain why you want to acquire with a specific id.

PeerXu commented 8 years ago

Yes, the second usecase almost like my usecase, but a litte difference is I need the lock with the same id, if not, get id from redis. Last, I think if Lock class have a argument with id, that we could new a Lock instance with id.

ionelmc commented 8 years ago

Ok, if that is indeed your usecase then we'd need to change some behaviors:

I'm not really sure about those changes ... some are quite problematic.

Can you explain why you need this? Like what exactly you intend to do in your application. I think I misunderstood what you actually need.

PeerXu commented 8 years ago

acquire would not do anything if it's already acquired with same id multiple waiting acquires with same id would all unblock at the same time

I think lock should block until other owner release that.

release would not do anything if already released

In my usecase, I will acquire the lock before release it, so I think raise exception is a better way.

and find a way to resolve option conflicts (like different expiry/renewal settings). What should happen if one process releases but other process still extends the lock?

It should not happen in my case.

I use id field to identify process. So I will create lock object with id field. Did I do the wrong way with it?