more info on thread local storage

3 files changed, 58 insertions, 9 deletions

diff --git a/CHANGES b/CHANGES
index 0f19282..32fce25 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,11 @@
+* 2.10.2 (in development)
+    * POSSIBLE BACKWARDS INCOMPATBLE CHANGE: Fixed a possible race condition
+      when multiple threads share the same Lock instance with a timeout. Lock
+      tokens are now stored in thread local storage by default. If you have
+      code that acquires a lock in one thread and passes that lock instance to
+      another thread to release it, you need to disable thread local storage.
+      Refer to the doc strings on the Lock class about the thread_local
+      argument information.
 * 2.10.1
     * Fixed a bug where Sentinel connections to a server that's no longer a
       master and receives a READONLY error will disconnect and reconnect to
diff --git a/redis/client.py b/redis/client.py
index ef360ff..03a86ab 100755
--- a/redis/client.py
+++ b/redis/client.py
@@ -478,7 +478,7 @@ class StrictRedis(object):
                     continue
 
     def lock(self, name, timeout=None, sleep=0.1, blocking_timeout=None,
-             lock_class=None, thread_local=False):
+             lock_class=None, thread_local=True):
         """
         Return a new Lock object using key ``name`` that mimics
         the behavior of threading.Lock.
@@ -498,10 +498,30 @@ class StrictRedis(object):
         ``lock_class`` forces the specified lock implementation.
 
         ``thread_local`` indicates whether the lock token is placed in
-        thread-local storage. Setting this to True may be necessary if
-        multiple execution contexts (such as threads or coroutines) share
-        a single Lock instance within a process. Defaults to False.
-        """
+        thread-local storage. By default, the token is placed in thread local
+        storage so that a thread only sees its token, not a token set by
+        another thread. Consider the following timeline:
+
+            time: 0, thread-1 acquires `my-lock`, with a timeout of 5 seconds.
+                     thread-1 sets the token to "abc"
+            time: 1, thread-2 blocks trying to acquire `my-lock` using the
+                     Lock instance.
+            time: 5, thread-1 has not yet completed. redis expires the lock
+                     key.
+            time: 5, thread-2 acquired `my-lock` now that it's available.
+                     thread-2 sets the token to "xyz"
+            time: 6, thread-1 finishes its work and calls release(). if the
+                     token is *not* stored in thread local storage, then
+                     thread-1 would see the token value as "xyz" and would be
+                     able to successfully release the thread-2's lock.
+
+        In some use cases it's necessary to disable thread local storage. For
+        example, if you have code where one thread acquires a lock and passes
+        that lock instance to a worker thread to release later. If thread
+        local storage isn't disabled in this case, the worker thread won't see
+        the token set by the thread that acquired the lock. Our assumption
+        is that these cases aren't common and as such default to using
+        thread local storage.        """
         if lock_class is None:
             if self._use_lua_lock is None:
                 # the first time .lock() is called, determine if we can use
diff --git a/redis/lock.py b/redis/lock.py
index 7d4a9b3..eea6c8b 100644
--- a/redis/lock.py
+++ b/redis/lock.py
@@ -15,7 +15,7 @@ class Lock(object):
     multiple clients play nicely together.
     """
     def __init__(self, redis, name, timeout=None, sleep=0.1,
-                 blocking=True, blocking_timeout=None, thread_local=False):
+                 blocking=True, blocking_timeout=None, thread_local=True):
         """
         Create a new Lock instance named ``name`` using the Redis client
         supplied by ``redis``.
@@ -41,9 +41,30 @@ class Lock(object):
         float or integer, both representing the number of seconds to wait.
 
         ``thread_local`` indicates whether the lock token is placed in
-        thread-local storage. Setting this to True may be necessary if
-        multiple execution contexts (such as threads or coroutines) share
-        a single Lock instance within a process. Defaults to False.
+        thread-local storage. By default, the token is placed in thread local
+        storage so that a thread only sees its token, not a token set by
+        another thread. Consider the following timeline:
+
+            time: 0, thread-1 acquires `my-lock`, with a timeout of 5 seconds.
+                     thread-1 sets the token to "abc"
+            time: 1, thread-2 blocks trying to acquire `my-lock` using the
+                     Lock instance.
+            time: 5, thread-1 has not yet completed. redis expires the lock
+                     key.
+            time: 5, thread-2 acquired `my-lock` now that it's available.
+                     thread-2 sets the token to "xyz"
+            time: 6, thread-1 finishes its work and calls release(). if the
+                     token is *not* stored in thread local storage, then
+                     thread-1 would see the token value as "xyz" and would be
+                     able to successfully release the thread-2's lock.
+
+        In some use cases it's necessary to disable thread local storage. For
+        example, if you have code where one thread acquires a lock and passes
+        that lock instance to a worker thread to release later. If thread
+        local storage isn't disabled in this case, the worker thread won't see
+        the token set by the thread that acquired the lock. Our assumption
+        is that these cases aren't common and as such default to using
+        thread local storage.
         """
         self.redis = redis
         self.name = name