Fixbug threading local in coroutine #120
Conversation
… of thread local mechanism being broken by coroutine
| @@ -77,8 +78,7 @@ def __init__(self, redis, name, timeout=None, sleep=0.1, | |||
| self.blocking = blocking | |||
| self.blocking_timeout = blocking_timeout | |||
| self.thread_local = bool(thread_local) | |||
| self.local = threading.local() if self.thread_local else dummy() | |||
| self.local.token = None | |||
| self.local = contextvars.ContextVar('token', default=None) if self.thread_local else dummy() | |||
There was a problem hiding this comment.
The documentation for contextvars has the following disclaimer:
Important: Context Variables should be created at the top module level and never in closures. Context objects hold strong references to context variables which prevents context variables from being properly garbage collected.
Declaring the context variable in the __init__ method may result in a memory leak if my understanding is correct. I propose to declare it outside of the class definition. For example:
token_var = contextvars.ContextVar('token', default=None)
class Lock:
def __init__(...):
self.local = token_var if self.thread_local else dummy()
...There was a problem hiding this comment.
On second thought, moving the contextvar definition to the module level will cause tokens to bleed between lock instances created by the same co-routine. That's a bad idea. Sorry, you can ignore my proposal.
There was a problem hiding this comment.
On second thought, moving the contextvar definition to the module level will cause tokens to bleed between lock instances created by the same co-routine. That's a bad idea. Sorry, you can ignore my proposal.
Sorry, github didn't send me a msg and i saw your code review so late.
| return is_error_raised | ||
|
|
||
| results = await asyncio.gather(coro(lock), coro(lock)) | ||
| assert not (results[0] and results[1]) |
There was a problem hiding this comment.
This assertion fails when I run this test locally. I believe the assertion is incorrect.
Both tasks/coroutines sleep for longer than the lifetime of the lock. As a consequence, both will encounter a LockError when attempting to release the lock. Hence, the correct assertion would be assert (result[0] and result[1])
There was a problem hiding this comment.
This assertion fails when I run this test locally. I believe the assertion is incorrect.
Both tasks/coroutines sleep for longer than the lifetime of the lock. As a consequence, both will encounter a LockError when attempting to release the lock. Hence, the correct assertion would be
assert (result[0] and result[1])
I think you are right, the test result is random which is not appropriate
changes:
* Fixbug: parsing stream messgae with empty payload will cause error(#116)
* Fixbug: Let ClusterConnectionPool handle skip_full_coverage_check (#118)
* New: threading local issue in coroutine, use contextvars instead of threading local in case of the safety of thread local mechanism being broken by coroutine (#120)
* New: support Python 3.8
Description
New: use contextvars instead of threading local in case of the safety of thread local mechanism being broken by coroutines
related issue #119