Skip to content

Happy Eyeballs crashes with Eager Task Factory #124309

Description

@mhils

Bug report

Bug description:

asyncio's staggered_race crashes when an eager task factory is being used:

import asyncio

async def main():
    asyncio.get_running_loop().set_task_factory(asyncio.eager_task_factory)
    await asyncio.open_connection("example.com", 80, happy_eyeballs_delay=0.25)

asyncio.run(main())
Traceback (most recent call last):
  [...]
  File "/tmp/repro.py", line 5, in main
    await asyncio.open_connection("example.com", 80, happy_eyeballs_delay=0.25)
  File "/opt/homebrew/Cellar/python@3.12/3.12.6/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/streams.py", line 48, in open_connection
    transport, _ = await loop.create_connection(
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.6/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py", line 1109, in create_connection
    sock, _, _ = await staggered.staggered_race(
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/python@3.12/3.12.6/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/staggered.py", line 144, in staggered_race
    raise d.exception()
  File "/opt/homebrew/Cellar/python@3.12/3.12.6/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/staggered.py", line 101, in run_one_coro
    assert len(running_tasks) == this_index + 2
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

See also: aio-libs/aiohttp#8599

CPython versions tested on:

3.12

Operating systems tested on:

macOS

Linked PRs

Activity

  1. added
    type-bugAn unexpected behavior, bug, or error
    on Sep 22, 2024
  2. rruuaanng commented on Sep 22, 2024

    @rruuaanng
    Contributor

    I'd love to help you, but what I can't understand is why it creates an extra task that triggers the assertion.

    next_task = loop.create_task(run_one_coro(this_failed))
    running_tasks.append(next_task)
    print('\n', 'HELLO', len(running_tasks), this_index)
    assert len(running_tasks) == this_index + 2
    # Prepare place to put this coroutine's exceptions if not won
    exceptions.append(None)
    assert len(exceptions) == this_index + 1
    
     HELLO 1 1
     HELLO 3 2
    

    Maybe you can start from here, sorry I'm not familiar with this.

  3. kumaraditya303 commented on Sep 22, 2024

    @kumaraditya303
    Contributor

    Probably removing that assert is sufficient.

  4. rruuaanng commented on Sep 22, 2024

    @rruuaanng
    Contributor

    Probably removing that assert is sufficient.

    Removing that assertion won't fix the issue; instead, it could cause new errors elsewhere. the real problem isn't with the assertion anyway.

  5. added
    3.12only security fixes
    3.13only security fixes
    3.14bugs and security fixes
    on Sep 23, 2024
  6. ZeroIntensity commented on Sep 23, 2024

    @ZeroIntensity
    Member

    This issue is unrelated to Happy Eyeballs, but instead that staggered_race doesn't support eager_task_factory. Seperate reproducer:

    import asyncio
    import random
    
    async def main():
        asyncio.get_running_loop().set_task_factory(asyncio.eager_task_factory)
        await asyncio.staggered.staggered_race([lambda: asyncio.sleep(0) for _ in range(5)], 0.25)
    
    asyncio.run(main())

    Using -O (to disable assertions) is a workaround for all versions, but maybe it's worth revisiting staggered_race a little bit to make sure it's working correctly with eager task factory. I'm working on that.

  7. ZeroIntensity commented on Sep 23, 2024

    @ZeroIntensity
    Member

    Ah, using delay=None in my reproducer causes a deadlock. Removing the assertion isn't 100% effective.

  8. ZeroIntensity commented on Sep 23, 2024

    @ZeroIntensity
    Member

    I've created #124390 as a fix. The existing implementation was so broken for eager task factories (due to the reliance of tasks not starting before you await them), so I had to pretty much rewrite it.

    Could you confirm that it solves your problem, @mhils?

  9. rruuaanng commented on Sep 24, 2024

    @rruuaanng
    Contributor

    I've created #124390 as a fix. The existing implementation was so broken for eager task factories (due to the reliance of tasks not starting before you await them), so I had to pretty much rewrite it.

    Could you confirm that it solves your problem, @mhils?

    That's right, I was confused when I tried to fix it, I kept thinking that maybe it was supporting eager_task_factory .

  10. 8 remaining items

  11. added a commit that references this issue on Oct 1, 2024
  12. moved this from Done to In Progress in asyncioon Oct 1, 2024
  13. ZeroIntensity commented on Oct 1, 2024

    @ZeroIntensity
    Member

    Sorry, we had to revert the fix due to the complexity of it (and also because it was causing some other problems). We'll (hopefully) have the fix for this issue in the next-next 3.12 patch (not tomorrow's, the one after, so 3.12.8).

  14. added a commit that references this issue on Oct 1, 2024
  15. added a commit that references this issue on Oct 11, 2024
  16. added 2 commits that reference this issue on Oct 11, 2024
  17. added 2 commits that reference this issue on Oct 12, 2024
  18. graingert commented on Oct 21, 2024

    @graingert
    Contributor

    I think this can be closed, it's backported to 3.12 and 3.13

  19. moved this from In Progress to Done in asyncioon Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    3.12only security fixes3.13only security fixes3.14bugs and security fixesstdlibStandard Library Python modules in the Lib/ directorytopic-asynciotype-bugAn unexpected behavior, bug, or error

    Projects

    • Status
      Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions