Merge pull request #18 from Joshuaalbert/fix-gh17

Fix #17

.github/workflows/unittests.yml

@@ -28,7 +28,7 @@ jobs:
         run: |
           python -m pip install --upgrade pip
           python -m pip install flake8 pytest
-          pip install -r requirements.txt
+          pip install -r requirements-tests.txt
           pip install .
       - name: Lint with flake8
         run: |

README.md

@@ -15,6 +15,12 @@ because [python decided not to support RLock in asyncio](https://discuss.python.
 their [argument](https://discuss.python.org/t/asyncio-rlock-reentrant-locks-for-async-python/21509/2) being that every
 extra bit of functionality adds to maintenance cost.
 
+Install with
+
+```bash
+pip install fair-async-rlock
+```
+
 ## About Fair Reentrant Lock for AsyncIO
 
 A reentrant lock (or recursive lock) is a particular type of lock that can be "locked" multiple times by the same task
@@ -100,4 +106,9 @@ reentrant lock if you have a specific need for it._**
 ### Performance
 
 The performance is about 50% slower than `asyncio.Lock`, i.e. overhead of sequential locks is about 3/2 of same
-with `asyncio.Lock`.
+with `asyncio.Lock`.
+
+### Change Log
+
+27 Jan, 2024 - 1.0.7 released. Fixed a bug that allowed another task to get the lock before a waiter got its turn on the
+event loop.

fair_async_rlock/fair_async_rlock.py

@@ -1,7 +1,9 @@
 import asyncio
 from collections import deque
 
-__all__ = ['FairAsyncRLock']
+__all__ = [
+    'FairAsyncRLock'
+]
 
 
 class FairAsyncRLock:
@@ -12,6 +14,7 @@ class FairAsyncRLock:
     def __init__(self):
         self._owner: asyncio.Task | None = None
         self._count = 0
+        self._owner_transfer = False
         self._queue = deque()
 
     def is_owner(self, task=None):
@@ -28,8 +31,8 @@ async def acquire(self):
             self._count += 1
             return
 
-        # If the lock is free, acquire it immediately
-        if self._count == 0:
+        # If the lock is free (and ownership not in midst of transfer), acquire it immediately
+        if self._count == 0 and not self._owner_transfer:
             self._owner = me
             self._count = 1
             return
@@ -41,12 +44,14 @@ async def acquire(self):
         # Wait for the lock to be free, then acquire
         try:
             await event.wait()
+            self._owner_transfer = False
             self._owner = me
             self._count = 1
         except asyncio.CancelledError:
-            try: # if in queue, then cancelled before release
+            try:  # if in queue, then cancelled before release
                 self._queue.remove(event)
-            except ValueError: # otherwise, release happened, this was next, and we simulate passing on
+            except ValueError:  # otherwise, release happened, this was next, and we simulate passing on
+                self._owner_transfer = False
                 self._owner = me
                 self._count = 1
                 self._current_task_release()
@@ -60,6 +65,8 @@ def _current_task_release(self):
                 # Wake up the next task in the queue
                 event = self._queue.popleft()
                 event.set()
+                # Setting this here prevents another task getting lock until owner transfer.
+                self._owner_transfer = True
 
     def release(self):
         """Release the lock"""

fair_async_rlock/tests/test_fair_async_rlock.py

@@ -375,15 +375,14 @@ async def task2():
             t2_ac.set()
             await asyncio.sleep(1.)  # Let's ensure the lock is held for a bit
 
-
     task1 = asyncio.create_task(task1())
     task2 = asyncio.create_task(task2())
     await asyncio.sleep(0.1)  # Yield control to allow tasks to start
     task2.cancel()
     with pytest.raises(asyncio.CancelledError):
         await task2
-    assert not t2_ac.is_set() # shouldn't acquire
-    t1_done.set() # Let T1 finish
+    assert not t2_ac.is_set()  # shouldn't acquire
+    t1_done.set()  # Let T1 finish
     await task1  # Ensure task1 has a chance to release the lock
     # Ensure that lock is not owned and queue is empty after cancellation
     assert lock._owner is None
@@ -609,3 +608,47 @@ async def task4():
     # Task 4 should not deadlock. It should be able to acquire the locks
     await asyncio.wait([t4], timeout=1)
     assert task4_acquired.is_set()
+
+
+@pytest.mark.asyncio
+async def test_gh17_regression():
+    lock = FairAsyncRLock()
+
+    # Use events to control the order of execution
+    task1_aquired = asyncio.Event()
+    # task1_released = asyncio.Event()
+    task2_acquired = asyncio.Event()
+
+    async def task1():
+        async with lock:
+            # tell task 2 to acquire lock
+            task1_aquired.set()
+            # but sleep long enough to make sure task 2 waits on lock before we release this one.
+            await asyncio.sleep(0.1)
+        # task1_released.set()
+
+    async def task2():
+        await task1_aquired.wait()
+        async with lock:
+            task2_acquired.set()
+            # hold this for long enough to allow task3 to release first in a race condition
+            # in which task3 beats task2 to ownership and then releases only to find
+            # task2 has set ownership, giving a foreign lock assert scenario
+            await asyncio.sleep(0.2)
+
+    async def task3():
+        # NB: we achieve this race condition with sleep(0.1)
+        # awaiting task1_released does not give it the edge
+        await asyncio.sleep(0.1)
+        # await task1_released.wait()
+        async with lock:
+            # if task 3 were to get an immediate lock (beating task3 to ownership), then waiting
+            # for this means we don't release until just after task2
+            # has clobbered over our ownership.
+            await task2_acquired.wait()
+
+    t1 = asyncio.create_task(task1())
+    t2 = asyncio.create_task(task2())
+    t3 = asyncio.create_task(task3())
+
+    await asyncio.gather(t1, t2, t3)

requirements.txt → requirements-tests.txt

@@ -1,2 +1,2 @@
-pytest
+pytest<8.0.0
 pytest-asyncio

setup.py

@@ -7,14 +7,15 @@
     long_description = fh.read()
 
 setup(name='fair_async_rlock',
-      version='1.0.6',
+      version='1.0.7',
       description='A well-tested implementation of a fair asynchronous RLock for concurrent programming.',
       long_description=long_description,
       long_description_content_type="text/markdown",
       url="https://github.com/Joshuaalbert/FairAsyncRLock",
       author='Joshua G. Albert',
       author_email='[email protected]',
       setup_requires=[],
+      install_requires=[],
       tests_require=[
           'pytest',
           'pytest-asyncio'