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
551 stars 77 forks source link

listing all locks #41

Closed marcstreeter closed 8 years ago

marcstreeter commented 8 years ago

I've been looking through the code and I don't see a way (short of using redis directly) to list all locks. Further not a way to filter locks based on owner. I'm already using this as a locking mechanism, but I want to be able to display locked items for visual feedback. I figure that instead of using say a hostname, we'd use the client id as the owner of the lock and then be able to display locks on a per client basis. With the current set up however it looks like each lock would have to be retrieved and then parsed to do so. Maybe I'm reading the source wrong or attempting to use this in the wrong way. Any advice would be appreciated.

ionelmc commented 8 years ago

Hmmm, interesting. Can you explain a bit more why you need this? Are you using custom lock ids? (the id arg)

AndreiPashkin commented 8 years ago

You can list all locks with Redis command KEYS lock:*

marcstreeter commented 8 years ago

Currently the locking done is all internal (i.e. only visible to the application not the user) and the name given the lock includes the id of the server such that "item_category_being_locked=" constitutes itself as a lock name. It both identifies what kind of lock is being done and the instance being locked. What I want to do is also provide the owner of the lock and be able to list all locks based on owner. I realize that the lock id is available as a way of showing 'ownership' for the lock, however it looks like what is being done is the lock name is the redis key and the lock id is the redis value. So to list all locks owned by a particular id I'd have to get all keys and only display those with a particular redis value. I can imagine this is doable in redis via lua, however, it doesn't seem like the way it would be done (do to the current setup) would be very efficient. Currently for instance in your code your reset all locks by using locks:* to access them all. It seems like it would be simple enough to structure locks in the same way locks:<owner_id>:<lock_name> such that all locks could be listed or scanned through using locks:23423:* (if, for instance, 23423 was the owner).

Of course more to the point the reason why I want to do this is more for a front end use of locks where a user has access to certain functionality based on the locks. Such that if something where locked then certain functionality would be restricted.

I dunno at this point I'm just rambling.

marcstreeter commented 8 years ago

I'm guessing that my implied proposal isn't in line with what you're planning, and that your suggested mode of action is to use KEYS lock:*?

ionelmc commented 8 years ago

Filtering by owner is a problem, because the id is usually automatically generated. You can use custom one but are you going to do all that amount of book-keeping?

I wanted to understand why you need this ...

marcstreeter commented 8 years ago

I want to provide relevant updates to a particular user's interface based on locks.

ionelmc commented 8 years ago

It sounds like you trying to use this as a result backend for a job queue. Am I misunderstanding?

marcstreeter commented 8 years ago

I'm not forming my sentences right perhaps. I don't mean to provide 'updates' per se, but rather update the interface based on locks. I already provide asynchronous results via celery's job queue. It so happens I already use this project in conjunction with celery to provide locking internally. I just really want to expose that locking to the user's interface.

ionelmc commented 8 years ago

What sort of resources are you locking?

marcstreeter commented 8 years ago

I think that is somewhat tangent to the issue, but to answer the question I'm locking many types of resources. In general they are cloud based resources (virtual networks, virtual servers, virtual storage, etc). To bring this answer back to the original question - every resource is tied to a user.

ionelmc commented 8 years ago

I think your usecase should be possible, but not as a core feature. There are already too many features :)

This could be implemented in a subclass I think. Do you use the expires or autoextend features?

ionelmc commented 8 years ago

Sorry, I meant the expire and auto_renewal

marcstreeter commented 8 years ago

I'm using the expire and auto_renewal feature. I may just fork this and try to implement it myself. If you're interested I can create pull request.

ionelmc commented 8 years ago

I won't mind a PR but I suspect testing is going to be a thorny problem :)

Regarding implementation, simply using different convention for key names won't work (like the locks:<owner_id>:<lock_name> you suggested) - everything operates on keynames that don't include (or allow) any id. You will need a third key and additional book-keeping for it.

marcstreeter commented 8 years ago

Thank you for the input, I'll keep it in mind and look for a way that would work.

ionelmc commented 8 years ago

A suggested approach (untested):

class UserLock(Lock):
  def __init__(self, redis, name, user, **kwargs):
    super(UserLock, self).__init__(redis, name, id=str(user), **kwargs)
  def acquire(self, blocking=True):
    locked = super(UserLock, self).acquire(blocking)
    if locked:
      for key in self._client.keys(self._name + ':*'): # cleanup any expired keys
        self._client.del(key)
      self._client.set(self._name + ':' + self._id, self._id, ex=self._expire)
  def extend(self, expire=None):
    super(UserLock, self).extend(expire)
    self._client.set(self._name + ':' + self._id, self._id, ex=expire or self._expire)
ionelmc commented 8 years ago

Then you can run KEYS lock:*:<userid> to get all them locks.

AndreiPashkin commented 8 years ago

@marcstreeter @ionelmc

Why not just do this?

class OwnedLock(Lock):
    def __init__(self, redis_client, name, *args, **kwargs):
        owner = kwargs.pop('owner', None)
        if owner is not None:
            name = name + ':' + owner
        super(OwnedLock, self).__init__(redis_client, name, *args, **kwargs)
ionelmc commented 8 years ago

It would not allow owners to share resources. I don't think that's what Marc wanted.

AndreiPashkin commented 8 years ago

@ionelmc What do you mean by sharing?

ionelmc commented 8 years ago

Two owners get owner-specific names, thus they cannot lock the same thing - because the name is not common between them.