Async lock can enter a deadlock if it is cancelled while being released

[liavt]

Introduction to the problem

Currently, the implementation of redis.asyncio.Lock can enter a deadlock state if release() is cancelled. This is due to the Lock resetting it's internal token before running the actual release function.

Simple reproduction code:

await lock.acquire()

release_task = asyncio.create_task(lock.release())
release_task.cancel()

try:
    await release_task
except asyncio.CancelledError:
    pass

await lock.locked() # => returns True
await lock.owned() # => returns False

As is shown by the final lines, Redis is still holding a entry for the lock, meaning no one can acquire it further. However, the lock object that acquired it thinks it doesn't own it, meaning any future attempts to call release() will raise a LockError that it is not owned.

In this state, it is impossible for any future lock on the same key to ever acquire it again. The only solution is manually intervening by deleting the key representing the lock.

Such a state is also easy to happen naturally, especially in highly async systems which rely on cancellation frequently.

Source of the problem

Looking at the code, we can see that it sets self.local.token = None before calling self.do_release.

redis-py/redis/asyncio/lock.py

Lines 274 to 275 in f026c1e

	self.local.token = None
	return self.do_release(expected_token)

Python's asyncio library will only raise a CancelledError when the next await is called after the task is cancelled, meaning that in the case of cancellation, the token will be set to None despite the actual release operation never occuring.

Proposed solution

I think the best solution would be to make release() be an async function instead of returning an Awaitable (which is an already strange typing.) In this case, it can run await self.do_release(expected_token) and only reset the token if the call succeeds.

For example:

await self.do_release(expected_token)
self.local.token = None

With this ordering, if do_release is cancelled, then the token will not be reset. Special care has to be given to the case where a LockNotOwnedError is raised if the lock is not owned, meaning the token should also be reset.

[petyaslavova]

Hi @liavt, thanks for bringing this up! We will have a look at it.