Repository: redis/redis-py
Pull request #3531 (2025-02-27 01:55:39)

feat: adds option not to raise exception when leaving context manager after lock expiration
Fixes: #3532 

### Pull Request check-list

_Please make sure to review and check all of these items:_

- [x] Do tests and lints pass with this change?
- [x] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- [x] Is the new or changed code fully tested?
- [x] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- [ ] Is there an example added to the examples folder (if applicable)?
- [ ] Was the change added to CHANGES file?

_NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open._

### Description of change

I was having trouble using async locks with the context manager because it was failing when exiting the context manager after timeout had expired (even if my code executed successfully).

I would really like not to raise a `LockNotOwnedError` in the exit of the context manager (if I try to release a lock and it was already "released" then great!).

I understand just changing the behavior would be critical, so I added an option to just behave this way when desired instead.

Issue: #3532 

julianolm: PTAL ?
@andymccurdy @dvora-h @chayim @petyaslavova @vladvildanov 
petyaslavova: Hi @julianolm, thank you for your contribution! Your change will be reviewed soon.
petyaslavova: Hi @julianolm, can you please check the reported errors from code linters?
julianolm: Hey @petyaslavova , updated!!! Thanks!
petyaslavova: @julianolm  you can run the linters locally - there are instructions in the contributing guide. 
Instead of using the "invoke all-tests" command, you can just call "invoke linters" and you will see all reported errors and changes that are needed.
julianolm: Oh, great. Sorry I missed it.
Now its ok.
Thanks again @petyaslavova 
julianolm: @petyaslavova wdyt?
petyaslavova: Hi @julianolm, you have conflicting change with the current master code and small change requested. Once these are resolved, we are ready to merge your change.
julianolm: Hey @petyaslavova thanks! Gonna address those today asap!
julianolm: Hello, @petyaslavova . Sorry for the delay. Busy day at work today.

Requests addressed. I felt it was better to catch both `LockNotOwnedError` and `LockError` separetely just so we can log more precise warnings on each situation.

But I will leave an opened suggestion for a single catch (of parent class `LockError`) if you think it is better. Please feel free to commit or discard it.

Thanks again!
julianolm: PS: I ran the tests with above suggestions applied and it works just fine as well
petyaslavova: > > But I will leave an opened suggestion for a single catch (of parent class `LockError`) if you think it is better. Please feel free to commit or discard it.
> 
> I left one warning - in case you just want to print a warning instead of failing, I don't think having different detailed messages will be so important.

@julianolm And now unfortunately, linters are failing ...
julianolm: Hello @petyaslavova linter problems solved

Patch:
diff --git a/redis/asyncio/client.py b/redis/asyncio/client.py
index 42544410..412d5a24 100644
--- a/redis/asyncio/client.py
+++ b/redis/asyncio/client.py
@@ -478,6 +478,7 @@ class Redis(
         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 @@ class Redis(
                      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 @@ class Redis(
             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 11d3f093..51dce895 100644
--- a/redis/asyncio/cluster.py
+++ b/redis/asyncio/cluster.py
@@ -839,6 +839,7 @@ class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommand
         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 @@ class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommand
                      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 @@ class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommand
             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 f70a8d09..16d7fb69 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 @@ from redis.typing import Number
 if TYPE_CHECKING:
     from redis.asyncio import Redis, RedisCluster
 
+logger = logging.getLogger(__name__)
+
 
 class Lock:
     """
@@ -85,6 +88,7 @@ class Lock:
         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 @@ class 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
@@ -144,6 +153,7 @@ class Lock:
         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 @@ class Lock:
         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 2bacbe14..2ba96bd6 100755
--- a/redis/client.py
+++ b/redis/client.py
@@ -473,6 +473,7 @@ class Redis(RedisModuleCommands, CoreCommands, SentinelCommands):
         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 @@ class Redis(RedisModuleCommands, CoreCommands, SentinelCommands):
                      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 @@ class Redis(RedisModuleCommands, CoreCommands, SentinelCommands):
             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 ef4500f8..c9523e2a 100644
--- a/redis/cluster.py
+++ b/redis/cluster.py
@@ -822,6 +822,7 @@ class RedisCluster(AbstractRedisCluster, RedisClusterCommands):
         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 @@ class RedisCluster(AbstractRedisCluster, RedisClusterCommands):
                      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 @@ class RedisCluster(AbstractRedisCluster, RedisClusterCommands):
             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 82f62730..bad447a0 100644
--- a/redis/exceptions.py
+++ b/redis/exceptions.py
@@ -89,7 +89,7 @@ class LockError(RedisError, ValueError):
 
 
 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 7a1becb3..0288496e 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 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 @@ class Lock:
         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 @@ class 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
@@ -140,6 +149,7 @@ class Lock:
         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 @@ class Lock:
         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,


Test patch:
diff --git a/tests/test_asyncio/test_lock.py b/tests/test_asyncio/test_lock.py
index 9973ef70..be4270ac 100644
--- a/tests/test_asyncio/test_lock.py
+++ b/tests/test_asyncio/test_lock.py
@@ -129,6 +129,36 @@ class TestLock:
             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 136c86e4..3d6d8146 100644
--- a/tests/test_lock.py
+++ b/tests/test_lock.py
@@ -133,6 +133,32 @@ class TestLock:
             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)