joshuaalbert / fairasyncrlock Goto Github PK

The release method is asynchronous, meaning it might be paused and control given back to the event loop at any point.

In the case of asyncio.Lock, the release method is synchronous, which guarantees that the lock is released immediately when release is called, and there is no chance for other async code to run until it completes.

Making release asynchronous in FairAsyncRLock might lead to some potential problems:

Note: There is no potential for race condition because we use Events to ensure ordering off granting.

Unexpected Exceptions: In a situation where the event loop is closed or there is an unhandled exception in another coroutine, the await self.release() in __aexit__ might not be executed or may be interrupted before it completes. This could leave the lock in an acquired state when it is expected to be released.

Cancellation and Deadlocks: If the task in which __aexit__ is being run is cancelled while it is awaiting self.release(), then self.release() might not finish executing. This could result in a deadlock if there is a task awaiting to acquire the lock, because it will be waiting for the lock to be released, but the task responsible for releasing the lock has been cancelled and will never release the lock.

Therefore, while making release asynchronous might seem more consistent with the rest of the async API, it can potentially introduce some complications that you wouldn't have if it were synchronous, like in asyncio.Lock. These issues might not be a problem in many cases, but they are potential edge cases to be aware of when designing asynchronous systems.

By looking at the code, I suspect there's a potential for dead locks. I will try to come up with a test case that shows that and will let you know if I succeed. Until then, you might want to think about it and see if you come to the same conclusion.

Sequence leading to a dead lock (assume 4 tasks that take turns, e.g., only ever await asyncio.sleep(0) and the event loop is round robin):

• Some task 1 acquires lock
• There is a task 2 doing some (so far unrelated) things
• Some task 3 tries to acquire the lock and ends up in the queue L39
• Task 3 awaits the event L43
• Some task 4 tries to acquire the lock (analog to the 2 steps above)
• Task 1 releases the lock and sets the event which task 3 is waiting on L57
• Now it's taks 2's turn again and it decides to cancel task 3 for whatever reason and then task 2 ends
• Task 3's turn: it was cancelled and removes itself from the queue L47 ⚠️ even though the event was set and task 3 was scheduled to take the lock
• Now it's task 4's turn... It will never be notified that the lock is actually free => ⚡ dead lock

Race condition:

when event.set() is called in the second snippet, another request to the lock can come in and immediately acquire it (first snippet), before the await event.wait() (third snippet) gets a chance to respond to the event.set(). So when this other event.wait() coro from the third snippet proceeds, it will clobber the owner and count for the immediately acquired coro from the first snippet. This leads to the exception f"Cannot release foreign lock. {me} tried to unlock {self._owner}." when the first snippet coro duly releases.

        # If the lock is free, acquire it immediately
        if self._count == 0:
            self._owner = me
            self._count = 1
            return

    def _current_task_release(self):
        self._count -= 1
        if self._count == 0:
            self._owner = None
            if self._queue:
                # Wake up the next task in the queue
                event = self._queue.popleft()
                event.set()

        try:
            await event.wait()
            self._owner = me
            self._count = 1

Describe the bug
The unit test, test_stochastic_cancellation(), simulates a scenario where multiple tasks are competing to acquire and release a lock in random order. Among those tasks, a monitor task will cancel a task randomly. The test asserts that at least one cancellation occurred.

The issue with the unit test is that there might be instances where the monitor task doesn't cancel any tasks because by the time it tries to cancel a task, all of the other tasks might have already completed.

Expected behavior
We always want at least one task to be cancelled.

Observed behavior
https://github.com/Joshuaalbert/FairAsyncRLock/actions/runs/5411884567/jobs/9835224169

FairAsyncLock version
1.0.4

This is a rather small note, but looking at __aexit__, there seems to be dead code.

    async def __aexit__(self, exc_type, exc, tb):
        try:
            self.release()
        except asyncio.CancelledError as e:
            if self.is_owner():
                # If a cancellation happened during release, we force the current task to release.
                self._current_task_release()
            raise e

Since self.release() isn't async, it can't be canceled. And looking at self.release(), it doesn't seem to generate asyncio.CancelledErrors itself. So the except block is not reachable, I think.