Use of threading.local in lock.py can cause token leakage

[eirikur-grid]

The use of threading.local()in lock.py appears to have been ported directly from redis-py over to this project. As far as I can tell, this can lead to tokens leaking between asynchronous tasks executed within the same event loop (i.e. on the same thread).

Let's consider the following scenario:

• Task A successfully acquires a lock using token a and a timeout of 3 seconds
• Task B requests the lock with blocking=True and blocking_timeout=None
• Task A takes longer than 3 seconds to finish and so the lock is automatically released
• Task B now successfully holds the lock with token b, which is placed in thread local storage
• Task A finishes and attempts to release the lock. It retrieves the token b from thread local storage and is successful in releasing the lock, as opposed to getting a LockError due to a token mismatch as would be the correct behavior.

[NoneGG]

@eirikur-grid Thanks for bug report, i will check it and give my feedback as soon as possible.

[NoneGG]

@eirikur-grid I am afraid you are right, different coroutines share the same thread, which will break the safety of thread local mechanism. In Python 3.7 i can use contextvars to fix this, i will see if there is a good solution in Python 3.6

[NoneGG]

@eirikur-grid
I found a backport of contextvars for 3.5/3.6package, this bug will be fixed soon, thanks again for bug report

[eirikur-grid]

Excellent.
Feel free to ping me if you want someone to look over the PR with the fix.

[NoneGG]

@eirikur-grid The fix is finished, would you like to see it ? It will be merged If there is no other problem :-)

[NoneGG]

@eirikur-grid Merged, thanks again for bug report~

[eirikur-grid]

@eirikur-grid The fix is finished, would you like to see it ? It will be merged If there is no other problem :-)

Sorry for the late reply. I somehow missed the e-mail notification. I'll take a post-merge look and attempt to verify the fix.

[eirikur-grid]

@NoneGG I have verified that the fix works as intended. However, I noticed that the newly added test is failing. See: https://github.com/NoneGG/aredis/pull/120/files#r317581785