releasing shared Lock instances with timeouts

[chillipino]

i didn't see a way to reopen #416, so i'm submitting a new issue. everything about the bug description is the same, so please read about it there. here is a gist demonstrating the problem.

as before, the problem only happens if the execution context that originally owned the lock runs over the time limit that the programmer set, so this could be considered a bug in calling code. it's really easy to add implicit sources of delays, though (any code that depends on TCP connections could time out), so i think it would be useful to give calling code a method to get around this.

the recommendation from #416 to prefix a string onto the token that distinguishes between calling contexts within a single process would still work, but i'm sure there are other ways of dealing with it. the prefix idea does require authors of calling code to be aware of the cases when the prefix must be used. i think it's better not to make any assumptions about the calling environment (for instance, assuming the current thread ID should be prepended).

if needed, i can do the work once an implementation direction is set.

[andymccurdy]

I wonder if we can somewhat cheat and make the Lock/LuaLock derive from threading.local. That way the token value would certainly be unique per thread.

I'm not sure if threading.local works the same way with gevent/eventlet coroutines though.

[andymccurdy]

It does appear that gevent at least has a threading.local concept and it gets used when monkey patching the stdlib.

[andymccurdy]

Switching Lock to derive from threading.local does seem to fix the issue:

('lock type is:', <class 'redis.lock.LuaLock'>)
0 waiting for lock
0 lock acquired
0 encounters a failure and takes too long
1 waiting for lock
1 lock acquired
1 doing work
0 releasing lock
Exception in thread Thread-1:
Traceback (most recent call last):
  File "/Users/andy/.pyenv/versions/2.7.6/lib/python2.7/threading.py", line 810, in __bootstrap_inner
    self.run()
  File "/Users/andy/.pyenv/versions/2.7.6/lib/python2.7/threading.py", line 763, in run
    self.__target(*self.__args, **self.__kwargs)
  File "<ipython-input-1-9f36eb7d28ab>", line 17, in thread0
    theLock.release()
  File "redis/lock.py", line 106, in release
    self.do_release(expected_token)
  File "redis/lock.py", line 235, in do_release
    raise LockError("Cannot release a lock that's no longer owned")
LockError: Cannot release a lock that's no longer owned

1 releasing lock

[chillipino]

seems like a reasonable idea, there are a lot of potential calling environments, though (i don't know what this would do in twisted, for instance), which is why i thought it would be better to let the author of the calling code decide how to handle it.

looks like threading.local puts a lock around every attribute access (see _threading_local.py). if you want to go forward with this, maybe it would be best to just put the token in a threading.local instance on the Lock?

[chillipino]

added a pull request with the token attribute moved to thread local storage. it does fix the problem in my test script. i'll try using this in my code (gunicorn/gevent) and report back if there are any problems.

[andymccurdy]

@chillipino looks like the test errors are all cases where the tests use lock.token to compare to the value stored in redis. we'd just need to change those references to lock.local.token instead.

[chillipino]

done. i actually ran the tests this time. :)

[andymccurdy]

@chillipino Assuming this works with gevent, I'm happy merging it in. Let me know how your tests go.

[chillipino]

@andymccurdy when i switch to the new code (and force the timeout condition), the error does change from "unlocked lock" to "no longer owned" as expected when using gunicorn/gevent. i didn't do a detailed check to make sure the error is being raised in the context that timed out, but the exception is raised at the right time in the gist, so i think this looks good.

i should note that i'm a bit behind on gevent versions - i'm still using 0.13.8, the last non-beta release before 1.0. the list of breaking changes doesn't appear to include anything that would obviously affect this.

[andymccurdy]

Great, I'll go ahead and merge this then. Thanks for the patch and testing it ;)

[andymccurdy]

merged