diff --git a/redis/asyncio/client.py b/redis/asyncio/client.py index 4254441073..412d5a24b3 100644 --- a/redis/asyncio/client.py +++ b/redis/asyncio/client.py @@ -478,6 +478,7 @@ def lock( blocking_timeout: Optional[float] = None, lock_class: Optional[Type[Lock]] = None, thread_local: bool = True, + raise_on_release_error: bool = True, ) -> Lock: """ Return a new Lock object using key ``name`` that mimics @@ -524,6 +525,11 @@ def lock( thread-1 would see the token value as "xyz" and would be able to successfully release the thread-2's lock. + ``raise_on_release_error`` indicates whether to raise an exception when + the lock is no longer owned when exiting the context manager. By default, + this is True, meaning an exception will be raised. If False, the warning + will be logged and the exception will be suppressed. + 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 @@ -541,6 +547,7 @@ def lock( blocking=blocking, blocking_timeout=blocking_timeout, thread_local=thread_local, + raise_on_release_error=raise_on_release_error, ) def pubsub(self, **kwargs) -> "PubSub": diff --git a/redis/asyncio/cluster.py b/redis/asyncio/cluster.py index 11d3f093f7..51dce89597 100644 --- a/redis/asyncio/cluster.py +++ b/redis/asyncio/cluster.py @@ -839,6 +839,7 @@ def lock( blocking_timeout: Optional[float] = None, lock_class: Optional[Type[Lock]] = None, thread_local: bool = True, + raise_on_release_error: bool = True, ) -> Lock: """ Return a new Lock object using key ``name`` that mimics @@ -885,6 +886,11 @@ def lock( thread-1 would see the token value as "xyz" and would be able to successfully release the thread-2's lock. + ``raise_on_release_error`` indicates whether to raise an exception when + the lock is no longer owned when exiting the context manager. By default, + this is True, meaning an exception will be raised. If False, the warning + will be logged and the exception will be suppressed. + 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 @@ -902,6 +908,7 @@ def lock( blocking=blocking, blocking_timeout=blocking_timeout, thread_local=thread_local, + raise_on_release_error=raise_on_release_error, ) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index f70a8d09ab..16d7fb6957 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -1,4 +1,5 @@ import asyncio +import logging import threading import uuid from types import SimpleNamespace @@ -10,6 +11,8 @@ if TYPE_CHECKING: from redis.asyncio import Redis, RedisCluster +logger = logging.getLogger(__name__) + class Lock: """ @@ -85,6 +88,7 @@ def __init__( blocking: bool = True, blocking_timeout: Optional[Number] = None, thread_local: bool = True, + raise_on_release_error: bool = True, ): """ Create a new Lock instance named ``name`` using the Redis client @@ -128,6 +132,11 @@ def __init__( thread-1 would see the token value as "xyz" and would be able to successfully release the thread-2's lock. + ``raise_on_release_error`` indicates whether to raise an exception when + the lock is no longer owned when exiting the context manager. By default, + this is True, meaning an exception will be raised. If False, the warning + will be logged and the exception will be suppressed. + 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 @@ -144,6 +153,7 @@ def __init__( self.blocking_timeout = blocking_timeout self.thread_local = bool(thread_local) self.local = threading.local() if self.thread_local else SimpleNamespace() + self.raise_on_release_error = raise_on_release_error self.local.token = None self.register_scripts() @@ -163,7 +173,14 @@ async def __aenter__(self): raise LockError("Unable to acquire lock within the time specified") async def __aexit__(self, exc_type, exc_value, traceback): - await self.release() + try: + await self.release() + except LockError: + if self.raise_on_release_error: + raise + logger.warning( + "Lock was unlocked or no longer owned when exiting context manager." + ) async def acquire( self, diff --git a/redis/client.py b/redis/client.py index 2bacbe14ac..2ba96bd6f9 100755 --- a/redis/client.py +++ b/redis/client.py @@ -473,6 +473,7 @@ def lock( blocking_timeout: Optional[float] = None, lock_class: Union[None, Any] = None, thread_local: bool = True, + raise_on_release_error: bool = True, ): """ Return a new Lock object using key ``name`` that mimics @@ -519,6 +520,11 @@ def lock( thread-1 would see the token value as "xyz" and would be able to successfully release the thread-2's lock. + ``raise_on_release_error`` indicates whether to raise an exception when + the lock is no longer owned when exiting the context manager. By default, + this is True, meaning an exception will be raised. If False, the warning + will be logged and the exception will be suppressed. + 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 @@ -536,6 +542,7 @@ def lock( blocking=blocking, blocking_timeout=blocking_timeout, thread_local=thread_local, + raise_on_release_error=raise_on_release_error, ) def pubsub(self, **kwargs): diff --git a/redis/cluster.py b/redis/cluster.py index ef4500f895..c9523e2a76 100644 --- a/redis/cluster.py +++ b/redis/cluster.py @@ -822,6 +822,7 @@ def lock( blocking_timeout=None, lock_class=None, thread_local=True, + raise_on_release_error: bool = True, ): """ Return a new Lock object using key ``name`` that mimics @@ -868,6 +869,11 @@ def lock( thread-1 would see the token value as "xyz" and would be able to successfully release the thread-2's lock. + ``raise_on_release_error`` indicates whether to raise an exception when + the lock is no longer owned when exiting the context manager. By default, + this is True, meaning an exception will be raised. If False, the warning + will be logged and the exception will be suppressed. + 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 @@ -885,6 +891,7 @@ def lock( blocking=blocking, blocking_timeout=blocking_timeout, thread_local=thread_local, + raise_on_release_error=raise_on_release_error, ) def set_response_callback(self, command, callback): diff --git a/redis/exceptions.py b/redis/exceptions.py index 82f62730ab..bad447a086 100644 --- a/redis/exceptions.py +++ b/redis/exceptions.py @@ -89,7 +89,7 @@ def __init__(self, message=None, lock_name=None): class LockNotOwnedError(LockError): - "Error trying to extend or release a lock that is (no longer) owned" + "Error trying to extend or release a lock that is not owned (anymore)" pass diff --git a/redis/lock.py b/redis/lock.py index 7a1becb30a..0288496e6f 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -1,3 +1,4 @@ +import logging import threading import time as mod_time import uuid @@ -7,6 +8,8 @@ from redis.exceptions import LockError, LockNotOwnedError from redis.typing import Number +logger = logging.getLogger(__name__) + class Lock: """ @@ -82,6 +85,7 @@ def __init__( blocking: bool = True, blocking_timeout: Optional[Number] = None, thread_local: bool = True, + raise_on_release_error: bool = True, ): """ Create a new Lock instance named ``name`` using the Redis client @@ -125,6 +129,11 @@ def __init__( thread-1 would see the token value as "xyz" and would be able to successfully release the thread-2's lock. + ``raise_on_release_error`` indicates whether to raise an exception when + the lock is no longer owned when exiting the context manager. By default, + this is True, meaning an exception will be raised. If False, the warning + will be logged and the exception will be suppressed. + 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 @@ -140,6 +149,7 @@ def __init__( self.blocking = blocking self.blocking_timeout = blocking_timeout self.thread_local = bool(thread_local) + self.raise_on_release_error = raise_on_release_error self.local = threading.local() if self.thread_local else SimpleNamespace() self.local.token = None self.register_scripts() @@ -168,7 +178,14 @@ def __exit__( exc_value: Optional[BaseException], traceback: Optional[TracebackType], ) -> None: - self.release() + try: + self.release() + except LockError: + if self.raise_on_release_error: + raise + logger.warning( + "Lock was unlocked or no longer owned when exiting context manager." + ) def acquire( self, diff --git a/tests/test_asyncio/test_lock.py b/tests/test_asyncio/test_lock.py index 9973ef701f..be4270acdf 100644 --- a/tests/test_asyncio/test_lock.py +++ b/tests/test_asyncio/test_lock.py @@ -129,6 +129,36 @@ async def test_context_manager_raises_when_locked_not_acquired(self, r): async with self.get_lock(r, "foo", blocking_timeout=0.1): pass + async def test_context_manager_not_raise_on_release_lock_not_owned_error(self, r): + try: + async with self.get_lock( + r, "foo", timeout=0.1, raise_on_release_error=False + ): + await asyncio.sleep(0.15) + except LockNotOwnedError: + pytest.fail("LockNotOwnedError should not have been raised") + + with pytest.raises(LockNotOwnedError): + async with self.get_lock( + r, "foo", timeout=0.1, raise_on_release_error=True + ): + await asyncio.sleep(0.15) + + async def test_context_manager_not_raise_on_release_lock_error(self, r): + try: + async with self.get_lock( + r, "foo", timeout=0.1, raise_on_release_error=False + ) as lock: + lock.release() + except LockError: + pytest.fail("LockError should not have been raised") + + with pytest.raises(LockError): + async with self.get_lock( + r, "foo", timeout=0.1, raise_on_release_error=True + ) as lock: + lock.release() + async def test_high_sleep_small_blocking_timeout(self, r): lock1 = self.get_lock(r, "foo") assert await lock1.acquire(blocking=False) diff --git a/tests/test_lock.py b/tests/test_lock.py index 136c86e459..3d6d81465e 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -133,6 +133,32 @@ def test_context_manager_raises_when_locked_not_acquired(self, r): with self.get_lock(r, "foo", blocking_timeout=0.1): pass + def test_context_manager_not_raise_on_release_lock_not_owned_error(self, r): + try: + with self.get_lock(r, "foo", timeout=0.1, raise_on_release_error=False): + time.sleep(0.15) + except LockNotOwnedError: + pytest.fail("LockNotOwnedError should not have been raised") + + with pytest.raises(LockNotOwnedError): + with self.get_lock(r, "foo", timeout=0.1, raise_on_release_error=True): + time.sleep(0.15) + + def test_context_manager_not_raise_on_release_lock_error(self, r): + try: + with self.get_lock( + r, "foo", timeout=0.1, raise_on_release_error=False + ) as lock: + lock.release() + except LockError: + pytest.fail("LockError should not have been raised") + + with pytest.raises(LockError): + with self.get_lock( + r, "foo", timeout=0.1, raise_on_release_error=True + ) as lock: + lock.release() + def test_high_sleep_small_blocking_timeout(self, r): lock1 = self.get_lock(r, "foo") assert lock1.acquire(blocking=False)