fix: update staggered race implementation to use latest cpython version

related aiohttp issue aio-libs/aiohttp#10506

In #101 we replaced the staggered race implementation since the cpython version had races
that were not fixed at the time. cpython has since updated the implementation to fix
additional races. Our current implementation still has problems with cancellation
and cpython has fixed that in python/cpython#128475
and python/cpython#124847

This PR ports the latest cpython implementation

Author: bdraco

src/aiohappyeyeballs/_staggered.py

@@ -1,14 +1,12 @@
 import asyncio
 import contextlib
 from typing import (
-    TYPE_CHECKING,
     Any,
     Awaitable,
     Callable,
     Iterable,
     List,
     Optional,
-    Set,
     Tuple,
     TypeVar,
     Union,
@@ -17,33 +15,6 @@
 _T = TypeVar("_T")
 
 
-def _set_result(wait_next: "asyncio.Future[None]") -> None:
-    """Set the result of a future if it is not already done."""
-    if not wait_next.done():
-        wait_next.set_result(None)
-
-
-async def _wait_one(
-    futures: "Iterable[asyncio.Future[Any]]",
-    loop: asyncio.AbstractEventLoop,
-) -> _T:
-    """Wait for the first future to complete."""
-    wait_next = loop.create_future()
-
-    def _on_completion(fut: "asyncio.Future[Any]") -> None:
-        if not wait_next.done():
-            wait_next.set_result(fut)
-
-    for f in futures:
-        f.add_done_callback(_on_completion)
-
-    try:
-        return await wait_next
-    finally:
-        for f in futures:
-            f.remove_done_callback(_on_completion)
-
-
 async def staggered_race(
     coro_fns: Iterable[Callable[[], Awaitable[_T]]],
     delay: Optional[float],
@@ -75,18 +46,16 @@ async def staggered_race(
             raise
 
     Args:
-    ----
         coro_fns: an iterable of coroutine functions, i.e. callables that
             return a coroutine object when called. Use ``functools.partial`` or
             lambdas to pass arguments.
 
         delay: amount of time, in seconds, between starting coroutines. If
             ``None``, the coroutines will run sequentially.
 
-        loop: the event loop to use. If ``None``, the running loop is used.
+        loop: the event loop to use.
 
     Returns:
-    -------
         tuple *(winner_result, winner_index, exceptions)* where
 
         - *winner_result*: the result of the winning coroutine, or ``None``
@@ -103,100 +72,113 @@ async def staggered_race(
           coroutine's entry is ``None``.
 
     """
+    # TODO: when we have aiter() and anext(), allow async iterables in coro_fns.
     loop = loop or asyncio.get_running_loop()
-    exceptions: List[Optional[BaseException]] = []
-    tasks: Set[asyncio.Task[Optional[Tuple[_T, int]]]] = set()
+    enum_coro_fns = enumerate(coro_fns)
+    winner_result: Optional[Any] = None
+    winner_index: Union[int, None] = None
+    unhandled_exceptions: list[BaseException] = []
+    exceptions: list[Union[BaseException, None]] = []
+    running_tasks: set[asyncio.Task[Any]] = set()
+    on_completed_fut: Union[asyncio.Future[None], None] = None
+
+    def task_done(task: asyncio.Task[Any]) -> None:
+        running_tasks.discard(task)
+        if (
+            on_completed_fut is not None
+            and not on_completed_fut.done()
+            and not running_tasks
+        ):
+            on_completed_fut.set_result(None)
+
+        if task.cancelled():
+            return
+
+        exc = task.exception()
+        if exc is None:
+            return
+        unhandled_exceptions.append(exc)  # noqa: F821 - defined in the outer scope
 
     async def run_one_coro(
-        coro_fn: Callable[[], Awaitable[_T]],
-        this_index: int,
-        start_next: "asyncio.Future[None]",
-    ) -> Optional[Tuple[_T, int]]:
-        """
-        Run a single coroutine.
-
-        If the coroutine fails, set the exception in the exceptions list and
-        start the next coroutine by setting the result of the start_next.
-
-        If the coroutine succeeds, return the result and the index of the
-        coroutine in the coro_fns list.
+        ok_to_start: asyncio.Event,
+        previous_failed: Union[None, asyncio.Event],
+    ) -> None:
+        # in eager tasks this waits for the calling task to append this task
+        # to running_tasks, in regular tasks this wait is a no-op that does
+        # not yield a future. See gh-124309.
+        await ok_to_start.wait()
+        # Wait for the previous task to finish, or for delay seconds
+        if previous_failed is not None:
+            with contextlib.suppress(asyncio.TimeoutError):
+                # Use asyncio.wait_for() instead of asyncio.wait() here, so
+                # that if we get cancelled at this point, Event.wait() is also
+                # cancelled, otherwise there will be a "Task destroyed but it is
+                # pending" later.
+                await asyncio.wait_for(previous_failed.wait(), delay)
+        # Get the next coroutine to run
+        try:
+            this_index, coro_fn = next(enum_coro_fns)
+        except StopIteration:
+            return
+        # Start task that will run the next coroutine
+        this_failed = asyncio.Event()
+        next_ok_to_start = asyncio.Event()
+        next_task = loop.create_task(run_one_coro(next_ok_to_start, this_failed))
+        running_tasks.add(next_task)
+        next_task.add_done_callback(task_done)
+        # next_task has been appended to running_tasks so next_task is ok to
+        # start.
+        next_ok_to_start.set()
+        # Prepare place to put this coroutine's exceptions if not won
+        exceptions.append(None)  # noqa: F821 - defined in the outer scope
+        assert len(exceptions) == this_index + 1  # noqa: F821, S101 - defined in the outer scope
 
-        If SystemExit or KeyboardInterrupt is raised, re-raise it.
-        """
         try:
             result = await coro_fn()
         except (SystemExit, KeyboardInterrupt):
             raise
         except BaseException as e:
-            exceptions[this_index] = e
-            _set_result(start_next)  # Kickstart the next coroutine
-            return None
-
-        return result, this_index
-
-    start_next_timer: Optional[asyncio.TimerHandle] = None
-    start_next: Optional[asyncio.Future[None]]
-    task: asyncio.Task[Optional[Tuple[_T, int]]]
-    done: Union[asyncio.Future[None], asyncio.Task[Optional[Tuple[_T, int]]]]
-    coro_iter = iter(coro_fns)
-    this_index = -1
+            exceptions[this_index] = e  # noqa: F821 - defined in the outer scope
+            this_failed.set()  # Kickstart the next coroutine
+        else:
+            # Store winner's results
+            nonlocal winner_index, winner_result
+            assert winner_index is None  # noqa: S101
+            winner_index = this_index
+            winner_result = result
+            # Cancel all other tasks. We take care to not cancel the current
+            # task as well. If we do so, then since there is no `await` after
+            # here and CancelledError are usually thrown at one, we will
+            # encounter a curious corner case where the current task will end
+            # up as done() == True, cancelled() == False, exception() ==
+            # asyncio.CancelledError. This behavior is specified in
+            # https://bugs.python.org/issue30048
+            current_task = asyncio.current_task(loop)
+            for t in running_tasks:
+                if t is not current_task:
+                    t.cancel()
+
+    propagate_cancellation_error = None
     try:
-        while True:
-            if coro_fn := next(coro_iter, None):
-                this_index += 1
-                exceptions.append(None)
-                start_next = loop.create_future()
-                task = loop.create_task(run_one_coro(coro_fn, this_index, start_next))
-                tasks.add(task)
-                start_next_timer = (
-                    loop.call_later(delay, _set_result, start_next) if delay else None
-                )
-            elif not tasks:
-                # We exhausted the coro_fns list and no tasks are running
-                # so we have no winner and all coroutines failed.
-                break
-
-            while tasks or start_next:
-                done = await _wait_one(
-                    (*tasks, start_next) if start_next else tasks, loop
-                )
-                if done is start_next:
-                    # The current task has failed or the timer has expired
-                    # so we need to start the next task.
-                    start_next = None
-                    if start_next_timer:
-                        start_next_timer.cancel()
-                        start_next_timer = None
-
-                    # Break out of the task waiting loop to start the next
-                    # task.
-                    break
-
-                if TYPE_CHECKING:
-                    assert isinstance(done, asyncio.Task)
-
-                tasks.remove(done)
-                if winner := done.result():
-                    return *winner, exceptions
+        ok_to_start = asyncio.Event()
+        first_task = loop.create_task(run_one_coro(ok_to_start, None))
+        running_tasks.add(first_task)
+        first_task.add_done_callback(task_done)
+        # first_task has been appended to running_tasks so first_task is ok to start.
+        ok_to_start.set()
+        propagate_cancellation_error = None
+        # Make sure no tasks are left running if we leave this function
+        while running_tasks:
+            on_completed_fut = loop.create_future()
+            try:
+                await on_completed_fut
+            except asyncio.CancelledError as ex:
+                propagate_cancellation_error = ex
+                for task in running_tasks:
+                    task.cancel(*ex.args)
+            on_completed_fut = None
+        if propagate_cancellation_error is not None:
+            raise propagate_cancellation_error
+        return winner_result, winner_index, exceptions
     finally:
-        # We either have:
-        #  - a winner
-        #  - all tasks failed
-        #  - a KeyboardInterrupt or SystemExit.
-
-        #
-        # If the timer is still running, cancel it.
-        #
-        if start_next_timer:
-            start_next_timer.cancel()
-
-        #
-        # If there are any tasks left, cancel them and than
-        # wait them so they fill the exceptions list.
-        #
-        for task in tasks:
-            task.cancel()
-            with contextlib.suppress(asyncio.CancelledError):
-                await task
-
-    return None, None, exceptions
+        del exceptions, propagate_cancellation_error, unhandled_exceptions

tests/test_staggered.py

@@ -47,11 +47,15 @@ async def coro(idx):
     await asyncio.sleep(0.1)
     loop.call_soon(finish.set_result, None)
     winner, index, excs = await task
-    assert len(winners) == 4
-    assert winners == [0, 1, 2, 3]
+    assert len(winners) == 1
+    assert winners == [0]
     assert winner == 0
     assert index == 0
-    assert excs == [None, None, None, None]
+    assert len(excs) == 4
+    assert excs[0] is None
+    assert isinstance(excs[1], asyncio.CancelledError)
+    assert isinstance(excs[2], asyncio.CancelledError)
+    assert isinstance(excs[3], asyncio.CancelledError)
 
 
 @pytest.mark.skipif(sys.version_info < (3, 12), reason="requires python3.12 or higher")
@@ -77,10 +81,14 @@ async def coro(idx):
         await asyncio.sleep(0.1)
         loop.call_soon(finish.set_result, None)
         winner, index, excs = await task
-        assert len(winners) == 4
-        assert winners == [0, 1, 2, 3]
+        assert len(winners) == 1
+        assert winners == [0]
         assert winner == 0
         assert index == 0
-        assert excs == [None, None, None, None]
+        assert len(excs) == 4
+        assert excs[0] is None
+        assert isinstance(excs[1], asyncio.CancelledError)
+        assert isinstance(excs[2], asyncio.CancelledError)
+        assert isinstance(excs[3], asyncio.CancelledError)
 
     loop.run_until_complete(run())

tests/test_staggered_cpython.py

@@ -141,6 +141,62 @@ async def main():
         loop.run_until_complete(main())
         loop.close()
 
+    async def test_multiple_winners(self):
+        event = asyncio.Event()
+
+        async def coro(index):
+            await event.wait()
+            return index
+
+        async def do_set():
+            event.set()
+            await asyncio.Event().wait()
+
+        winner, index, excs = await staggered_race(
+            [
+                lambda: coro(0),
+                lambda: coro(1),
+                do_set,
+            ],
+            delay=0.1,
+        )
+        self.assertIs(winner, 0)
+        self.assertIs(index, 0)
+        self.assertEqual(len(excs), 3)
+        self.assertIsNone(excs[0], None)
+        self.assertIsInstance(excs[1], asyncio.CancelledError)
+        self.assertIsInstance(excs[2], asyncio.CancelledError)
+
+    async def test_cancelled(self):
+        log = []
+        with self.assertRaises(TimeoutError):
+            async with asyncio.timeout(None) as cs_outer, asyncio.timeout(
+                None
+            ) as cs_inner:
+
+                async def coro_fn():
+                    cs_inner.reschedule(-1)
+                    await asyncio.sleep(0)
+                    try:
+                        await asyncio.sleep(0)
+                    except asyncio.CancelledError:
+                        log.append("cancelled 1")
+
+                    cs_outer.reschedule(-1)
+                    await asyncio.sleep(0)
+                    try:
+                        await asyncio.sleep(0)
+                    except asyncio.CancelledError:
+                        log.append("cancelled 2")
+
+                try:
+                    await staggered_race([coro_fn], delay=None)
+                except asyncio.CancelledError:
+                    log.append("cancelled 3")
+                    raise
+
+        self.assertListEqual(log, ["cancelled 1", "cancelled 2", "cancelled 3"])
+
 
 if __name__ == "__main__":
     unittest.main()