Key name used by Lock doesn't comply with the make_key pattern used by Django's cache backend

[jweyrich]

Sorry for bothering you so much :-)

I noticed the key name used by the Lock implementation is built by hand using a simple string concatenation, specifically 'lock:'+name. This of course creates a key with this exact name, which at first seems to be the Correct Thing To Do, however, the Django cache backend employs a more convoluted logic - see Cache key transformation, which generates a key name like :1:lock:name, where 1 is a version that can be specified when creating the key, and defaults to 1 if none is informed. The backend relies on a make_key function that by default joins a few parts (key_prefix, version, key).

This brings a few complications when creating a lock and further querying its value. When we create it, its name is just lock:foobar, but when we query it using cache.get('foobar') or cache.get('lock:foobar'), it will actually query ':<version>:<name>' instead.

A few suggestions that could work (I think):

• Expose the name property on the Lock (yet, we also have signal name). Doesn't seem like a good idea though;
• Expose a key_func property on the Lock class and invoke the function when generating the key name(s);
• Add a kwarg to Lock.init to pass the backend instance so that it can use the exact make_key implementation (reset_all would also need it) used by the configured backend. This would act according to the configured BACKEND in settings, which seems like a good idea. This seems to require changes in the RedisCache.lock (from django_cache.py) method as well, otherwise we'd have to extend the RedisCache to pass the backend to the lock instance being created.

Do you have a simpler idea?

[ionelmc]

Hard to say at first look, but few observations:

• We can really work with only few kinds of key_func - that only prefix the key. Given this, it would make sense to intentionally cripple the custom cache backend to only use the default_key_func. Or at least emit a warning if something else is used.
• On the other hand, the internal name and signal keys are, well, internal :) Could there be a different API that solves your problem? Eg: An API to check if a lock is held.
• Also, you could specify a custom 'KEY_FUNCTION': lambda key: key in your setting and then you have less of a problem.

[jweyrich]

I agree with the first 2 points. Specifically, the 2nd seems like a great idea :-)
The last one is a bit of a problem because people might have (and I do have) other cache backends, so changing KEY_FUNCTION would impact on them as well.

[ionelmc]

Well then, what API do you need? To check if lock is held? How about an .is_locked() method?

Also, what do you think about making a PR? 😁 Just pip install tox and then run with your target platform, eg tox -e 2.7-1.7 (for python 2.7 and django 1.7). Travis will run the rest.

[jweyrich]

I really appreciate all the time you have invested in answering my reports :-)
Right, is_locked would be useful, but I'd also need a way to query the lock value. Maybe query_lock or query_lock_value, something along this line. Regarding the PR, my python-foo is still a bit weak - I should say I'm stuck on the yellow belt (not to say white, sadly), but I can try to come up with a solution - won't be able to send a PR today though. Thanks again!

[ionelmc]

is_locked() could return the token. Maybe show some example code of how you
would use this? Hard to picture it otherwise.

Thanks,
-- Ionel Cristian Mărieș, blog.ionelmc.ro

On Wed, Dec 24, 2014 at 3:12 PM, Jardel Weyrich notifications@github.com
wrote:

I really appreciate all the time you have invested in answering my issues.
Right, is_locked would be useful, but I'd also need a way to query the
lock value. Maybe query_lock or query_lock_value, something along this
line. Regarding the PR, my python-foo is still a bit weak - I could say I'm
stuck on the yellow bel (not say white, sadly), but I can try to come up
with a solution - won't be able to send a PR today though. Thanks again!

—
Reply to this email directly or view it on GitHub
#10 (comment)
.

[jweyrich]

This would be a simpler version of my usage:

lock_value = cache.query_lock(lock_name)

if lock_value != None: # Lock already acquired?
    if lock_value == self.aws_instance_id: # By me?
        logger.info('The lock is already mine')
        # ...
    else:
        logger.info('The lock belongs to someone else')
        # ...
else: # Lock not yet acquired?
    lock = cache.lock(lock_name, expire=lock_ttl)
    lock.token = self.aws_instance_id

    if lock.acquire(blocking=False): # Try to acquire it!
        logger.info('I acquired the lock')
        # ...
    else:
        logger.info('Someone else got the lock faster than me')
        # ...

[jweyrich]

What do you think of the following?

diff --git a/src/redis_lock/__init__.py b/src/redis_lock/__init__.py
index 725b4a6..28557da 100644
--- a/src/redis_lock/__init__.py
+++ b/src/redis_lock/__init__.py
@@ -52,6 +52,17 @@ class Lock(object):
     def token(self):
         return urandom(16)

+    def query(self, default=None):
+        try:
+            value = self._client.get(self._name)
+        except e:
+            raise
+
+        if value is None:
+            return default
+
+        return value
+
     def acquire(self, blocking=True):
         logger.debug("Getting %r ...", self._name)

diff --git a/src/redis_lock/django_cache.py b/src/redis_lock/django_cache.py
index e310344..4ce7236 100644
--- a/src/redis_lock/django_cache.py
+++ b/src/redis_lock/django_cache.py
@@ -19,6 +19,9 @@ class RedisCache(PlainRedisCache):
     def lock(self, key, expire=None):
         return Lock(self.__client, key, expire=expire)

+    def query_lock(self, lock, default=None):
+        return lock.query(default=default)
+
     def reset_all(self):
         """
         Forcibly deletes all locks if its remains (like a crash reason). Use this with care.

Then I'd change my sample to something like:

lock = cache.lock(lock_name, expire=lock_ttl)
lock.token = self.aws_instance_id
lock_value = cache.query_lock(lock)

if lock_value != None: # Lock already acquired?
    if lock_value == self.aws_instance_id: # By me?
        ...

[ionelmc]

Few observations:

• The query_lock is redundant, you can do lock.query() just as well and it's less code.
• Somehow the name of the method needs to tell the user what it's doing or what it is for. query is a bit too generic for that. Perhaps get_owner or just owner? Maybe keeper? holder? I know this seems nit-picky but APIs with good naming need little documentation and are intuitive (highly desirable IMO).
• What would you use the default argument for?
• Also, about your example, would this be nicer?

else: # Lock not yet acquired?
    lock = cache.lock(lock_name, expire=lock_ttl, token=self.aws_instance_id)
    if lock.acquire(blocking=False): # Try to acquire it!
        logger.info('I acquired the lock')
        # ...
    else:
        logger.info('Someone else got the lock faster than me')
        # ...

Ideally token would be a readonly property to avoid corruption after lock is acquired.

[jweyrich]

The query_lock is redundant, you can do lock.query() just as well and it's less code.

Agreed. It's way simpler.

Perhaps get_owner or just owner? Maybe keeper? holder?

If the library is going to assume people will use the lock value to store the holder/owner identifier (that token), I'm okay with it, otherwise these names won't make much sense.

What would you use the default argument for?

Since the key/value should be handled as a lock, not a common key/value, the default argument doesn't make much sense indeed. I just got rid of it. Didn't put much thought.

Also, about your example, would this be nicer?

Definitely!

[ionelmc]

If the library is going to assume people will use the lock value to store the holder/owner identifier (that token), I'm okay with it, otherwise these names won't make much sense.

Now that I think about it more, id would be better than token? get_owner_id or get_owner_token ?

[jweyrich]

If we're going to make that assumption on how people will use it, I believe get_owner_id would be the more obvious.
One may still want to use the value to store another kind of data - but that's not my case.

[ionelmc]

Yes, get_owner_id and id instead of token seems best then. Will you do it? Tit for tat as they say 😁

[jweyrich]

Not sure I got your comment right. You meant rename _tok to _id and token to get_owner_id? Please, would you take a look at jweyrich@503b5e4 and tell me what you'd like to change? Will try to update the docs (and probably tests) afterwards.

[ionelmc]

Looks good, make it a pull request. I've added some notes about the id property on your commit.

[jweyrich]

Done in jweyrich@49a5e0e - amended my previous commit and force-pushed it over the previous one.

[ionelmc]

Now it only needs some tests with the custom id :)

[jweyrich]

So, here's a user that's using tox for the 1st time - thanks for the introduction :-)

I can't seem to get it to skip those hundreds of tests from Django==1.7.1. It fails at ~1/2 of them and seem to abort before running the redis_lock tests. Any hints? The tox documentation didn't help much (to skip them), yet.

468 failed, 442 passed, 271 skipped, 1 xfailed, 16 warnings, 9 error in 73.29 seconds

[ionelmc]

Tha's strange, it shouldn't collect the django tests. How did you run it?

[jweyrich]

tox -e 2.7-1.7 as you suggested previously. Unfortunately I will be off until tomorrow (from now), but I'll take a look whenever I can. Thank you for now :-)

[ionelmc]

Not sure what to say, you should commit your tests so I can take a look.

[ionelmc]

Can you make another PR with a bit of explanation about the new api in the README.rst?

[jweyrich]

Done in #12. If you feel like the text or the sample needs to be changed, please, feel free ;-) Thank you again!