From 6d2bffe9d90358ece7754f1ab763316313badd0c Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Wed, 26 Feb 2025 22:45:44 -0300 Subject: [PATCH 01/10] adds option not to raise when leaving context manager after lock expiration --- redis/asyncio/client.py | 7 +++++++ redis/asyncio/lock.py | 18 +++++++++++++++++- redis/client.py | 7 +++++++ redis/exceptions.py | 2 +- redis/lock.py | 17 ++++++++++++++++- tests/test_asyncio/test_lock.py | 15 +++++++++++++++ tests/test_lock.py | 15 +++++++++++++++ 7 files changed, 78 insertions(+), 3 deletions(-) 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/lock.py b/redis/asyncio/lock.py index f70a8d09ab..334772f85f 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -1,6 +1,7 @@ import asyncio import threading import uuid +import logging from types import SimpleNamespace from typing import TYPE_CHECKING, Awaitable, Optional, Union @@ -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 @@ -127,6 +131,11 @@ def __init__( 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. + + ``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 @@ -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,13 @@ 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 LockNotOwnedError as e: + if self.raise_on_release_error: + raise e + logger.warning("Lock was 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/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..1aa5315f1f 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -1,12 +1,15 @@ import threading import time as mod_time import uuid +import logging from types import SimpleNamespace, TracebackType from typing import Optional, Type 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,12 @@ def __exit__( exc_value: Optional[BaseException], traceback: Optional[TracebackType], ) -> None: - self.release() + try: + self.release() + except LockNotOwnedError as e: + if self.raise_on_release_error: + raise e + logger.warning("Lock was 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..6f45bae9e7 100644 --- a/tests/test_asyncio/test_lock.py +++ b/tests/test_asyncio/test_lock.py @@ -129,6 +129,21 @@ 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_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_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..5888050cdd 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -133,6 +133,21 @@ 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_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_high_sleep_small_blocking_timeout(self, r): lock1 = self.get_lock(r, "foo") assert lock1.acquire(blocking=False) From 7f8de8d06015ad87b0260174735ac060913eef38 Mon Sep 17 00:00:00 2001 From: Juliano Amadeu <65794514+julianolm@users.noreply.github.com> Date: Thu, 27 Feb 2025 06:52:53 -0300 Subject: [PATCH 02/10] keep oroginal traceback Co-authored-by: Aarni Koskela --- redis/asyncio/lock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index 334772f85f..f368773037 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -177,7 +177,7 @@ async def __aexit__(self, exc_type, exc_value, traceback): await self.release() except LockNotOwnedError as e: if self.raise_on_release_error: - raise e + raise logger.warning("Lock was no longer owned when exiting context manager.") From cac17f43ceaedd11256172c6f9fc7b62491139d3 Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Thu, 27 Feb 2025 09:28:56 -0300 Subject: [PATCH 03/10] improves error traceback --- redis/asyncio/lock.py | 5 ++--- redis/lock.py | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index f368773037..f6c66c14ad 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -131,7 +131,7 @@ def __init__( 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. - + ``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 @@ -175,12 +175,11 @@ async def __aenter__(self): async def __aexit__(self, exc_type, exc_value, traceback): try: await self.release() - except LockNotOwnedError as e: + except LockNotOwnedError: if self.raise_on_release_error: raise logger.warning("Lock was no longer owned when exiting context manager.") - async def acquire( self, blocking: Optional[bool] = None, diff --git a/redis/lock.py b/redis/lock.py index 1aa5315f1f..c8f41ecb76 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -180,9 +180,9 @@ def __exit__( ) -> None: try: self.release() - except LockNotOwnedError as e: + except LockNotOwnedError: if self.raise_on_release_error: - raise e + raise logger.warning("Lock was no longer owned when exiting context manager.") def acquire( From a0f2a50a9b10845945e3394683e2b59b018d83ba Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Fri, 28 Feb 2025 14:22:39 -0300 Subject: [PATCH 04/10] adds missing modifications --- redis/asyncio/cluster.py | 7 +++++++ redis/cluster.py | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/redis/asyncio/cluster.py b/redis/asyncio/cluster.py index f343e26b75..ec1963920e 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/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): From 4c928c8c4b620467ac20d666e2775517a5f6731c Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Fri, 28 Feb 2025 14:26:20 -0300 Subject: [PATCH 05/10] sort imports --- redis/asyncio/lock.py | 2 +- redis/lock.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index f6c66c14ad..933144cf5c 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -1,7 +1,7 @@ import asyncio +import logging import threading import uuid -import logging from types import SimpleNamespace from typing import TYPE_CHECKING, Awaitable, Optional, Union diff --git a/redis/lock.py b/redis/lock.py index c8f41ecb76..0d57ffc08e 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -1,7 +1,7 @@ +import logging import threading import time as mod_time import uuid -import logging from types import SimpleNamespace, TracebackType from typing import Optional, Type From a7b9d78798607d5fb98cc655c463def2d7c5fe5c Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Fri, 28 Feb 2025 14:51:23 -0300 Subject: [PATCH 06/10] run linter --- tests/test_lock.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/test_lock.py b/tests/test_lock.py index 5888050cdd..0f74c67756 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -135,17 +135,13 @@ def test_context_manager_raises_when_locked_not_acquired(self, r): def test_context_manager_not_raise_on_release_error(self, r): try: - with self.get_lock( - r, "foo", timeout=0.1, raise_on_release_error=False - ): + 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 - ): + with self.get_lock(r, "foo", timeout=0.1, raise_on_release_error=True): time.sleep(0.15) def test_high_sleep_small_blocking_timeout(self, r): From 1bfb7e03bed8994e44436a21a39aa515dee01b1d Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Fri, 7 Mar 2025 21:34:17 -0300 Subject: [PATCH 07/10] adds catch for other possible exception --- redis/asyncio/lock.py | 4 ++++ redis/lock.py | 4 ++++ tests/test_asyncio/test_lock.py | 17 ++++++++++++++++- tests/test_lock.py | 17 ++++++++++++++++- 4 files changed, 40 insertions(+), 2 deletions(-) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index 933144cf5c..ad7ace91b4 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -179,6 +179,10 @@ async def __aexit__(self, exc_type, exc_value, traceback): if self.raise_on_release_error: raise logger.warning("Lock was no longer owned when exiting context manager.") + except LockError: + if self.raise_on_release_error: + raise + logger.warning("Lock was unlocked when exiting context manager.") async def acquire( self, diff --git a/redis/lock.py b/redis/lock.py index 0d57ffc08e..edb7374722 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -184,6 +184,10 @@ def __exit__( if self.raise_on_release_error: raise logger.warning("Lock was no longer owned when exiting context manager.") + except LockError: + if self.raise_on_release_error: + raise + logger.warning("Lock was unlocked when exiting context manager.") def acquire( self, diff --git a/tests/test_asyncio/test_lock.py b/tests/test_asyncio/test_lock.py index 6f45bae9e7..be4270acdf 100644 --- a/tests/test_asyncio/test_lock.py +++ b/tests/test_asyncio/test_lock.py @@ -129,7 +129,7 @@ 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_error(self, r): + 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 @@ -144,6 +144,21 @@ async def test_context_manager_not_raise_on_release_error(self, r): ): 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 0f74c67756..3d6d81465e 100644 --- a/tests/test_lock.py +++ b/tests/test_lock.py @@ -133,7 +133,7 @@ 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_error(self, r): + 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) @@ -144,6 +144,21 @@ def test_context_manager_not_raise_on_release_error(self, r): 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) From 5aaf9db25651c9ea06f2afedec9be7b07f1d4f30 Mon Sep 17 00:00:00 2001 From: petyaslavova Date: Mon, 10 Mar 2025 10:17:44 +0200 Subject: [PATCH 08/10] Update redis/lock.py to catch Both LockNotOwnedError and LockError in one except statement as LockError. Co-authored-by: Juliano Amadeu <65794514+julianolm@users.noreply.github.com> --- redis/lock.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/redis/lock.py b/redis/lock.py index edb7374722..7cc53af34c 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -180,14 +180,10 @@ def __exit__( ) -> None: try: self.release() - except LockNotOwnedError: - if self.raise_on_release_error: - raise - logger.warning("Lock was no longer owned when exiting context manager.") except LockError: if self.raise_on_release_error: raise - logger.warning("Lock was unlocked when exiting context manager.") + logger.warning("Lock was unlocked or no longer owned when exiting context manager.") def acquire( self, From 3484fd29a293badd4ce632d21293b2ebe8474f0f Mon Sep 17 00:00:00 2001 From: petyaslavova Date: Mon, 10 Mar 2025 10:18:59 +0200 Subject: [PATCH 09/10] Update redis/asyncio/lock.py Co-authored-by: Juliano Amadeu <65794514+julianolm@users.noreply.github.com> --- redis/asyncio/lock.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index ad7ace91b4..8a44801132 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -175,14 +175,10 @@ async def __aenter__(self): async def __aexit__(self, exc_type, exc_value, traceback): try: await self.release() - except LockNotOwnedError: - if self.raise_on_release_error: - raise - logger.warning("Lock was no longer owned when exiting context manager.") except LockError: if self.raise_on_release_error: raise - logger.warning("Lock was unlocked when exiting context manager.") + logger.warning("Lock was unlocked or no longer owned when exiting context manager.") async def acquire( self, From c1a55c0a57584f86205c9cf20472df7b3d74e875 Mon Sep 17 00:00:00 2001 From: Juliano Moura Date: Mon, 10 Mar 2025 05:36:17 -0300 Subject: [PATCH 10/10] fix linter errors --- redis/asyncio/lock.py | 4 +++- redis/lock.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/redis/asyncio/lock.py b/redis/asyncio/lock.py index 8a44801132..16d7fb6957 100644 --- a/redis/asyncio/lock.py +++ b/redis/asyncio/lock.py @@ -178,7 +178,9 @@ async def __aexit__(self, exc_type, exc_value, traceback): except LockError: if self.raise_on_release_error: raise - logger.warning("Lock was unlocked or no longer owned when exiting context manager.") + logger.warning( + "Lock was unlocked or no longer owned when exiting context manager." + ) async def acquire( self, diff --git a/redis/lock.py b/redis/lock.py index 7cc53af34c..0288496e6f 100644 --- a/redis/lock.py +++ b/redis/lock.py @@ -183,7 +183,9 @@ def __exit__( except LockError: if self.raise_on_release_error: raise - logger.warning("Lock was unlocked or no longer owned when exiting context manager.") + logger.warning( + "Lock was unlocked or no longer owned when exiting context manager." + ) def acquire( self,