Using eager task factory with aiohttp 3.10.0 and cpython 3.12+ raise an AssertionError in stdlib asyncio.staggered (fixed in aiohappyeyeballs 2.4.3+)
#8599
Comments
|
This looks like a bug in stdlib asyncio.staggered Have you had a chance to look through python’s issue tracker? |
|
Are you by chance using an eager task factory? |
|
Yes, I'm using eager task factory: loop = asyncio.get_running_loop()
loop.set_task_factory(asyncio.eager_task_factory)aiohttp 3.9.5 worked fine with the same Python 3.12 and the eager task factory, 3.10 is triggering the bug. |
|
It looks like asyncio.staggered is broken with eager task factory because it assumes the task will not run immediately. Would you please open a bug report for cpython? |
|
A workaround would be to turn off happyeyeballs in the connector until cpython is fixed when using eager task factory.
|
bdraco
changed the title
bdraco
changed the title
asyncio.staggered
|
@2-5 I'd like to get this fixed in upstream cpython. Would you please open an issue there? |
|
We seem to have a somewhat related issue, only on python 3.11.9 (aiohttp 3.10.1) Traceback (most recent call last):
File "/usr/local/lib/python3.11/site-packages/aiobotocore/httpsession.py", line 208, in send
response = await self._session.request(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/aiohttp/client.py", line 648, in _request
conn = await self._connector.connect(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/aiohttp/connector.py", line 546, in connect
proto = await self._create_connection(req, traces, timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/aiohttp/connector.py", line 954, in _create_connection
_, proto = await self._create_direct_connection(req, traces, timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/aiohttp/connector.py", line 1282, in _create_direct_connection
transp, proto = await self._wrap_create_connection(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/aiohttp/connector.py", line 1036, in _wrap_create_connection
sock = await aiohappyeyeballs.start_connection(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/aiohappyeyeballs/impl.py", line 89, in start_connection
sock, _, _ = await staggered.staggered_race(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/asyncio/staggered.py", line 144, in staggered_race
raise d.exception()
File "/usr/local/lib/python3.11/asyncio/staggered.py", line 116, in run_one_coro
assert winner_index is None
^^^^^^^^^^^^^^^^^^^^
AssertionError |
|
@bdraco Sorry, I wouldn't know how to report this bug upstream. I don't know what a |
|
Also getting AssertionError from UPD: upgrading to Python 3.12.3 solved the issue? |
|
@gediminasel upgrading to Python 3.12.6 appears to have resolved the |
|
Upstream issue has been fixed, you guys should be able to get this working whenever the next 3.12 patch is released. In the meantime, you can build 3.12 from source to get the fix. |
|
Thank you @ZeroIntensity |
Tuesday: https://peps.python.org/pep-0693/ There's no action for us to take here, right? We can just close this issue. |
|
The end goal is to hopefully merge Thanks for the fix. |
bdraco
changed the title
asyncio.staggered (fixed in cpython 3.12.7, aiohappyeyeballs 2.4.1)asyncio.staggered (fixed in cpython 3.12.7, aiohappyeyeballs 2.4.1 OR aiohappyeyeballs 2.4.2+)
|
It looks like the test failures with the new code and 3.13 are because of a change in the cancellation semantics via python/cpython#117407 |
|
Oh boy, that doesn't sound good. This is why it's a bad idea to rely on undocumented APIs. We're working on fixing the backwards compatibility issue via #124700, but as far as I can see, #117407 did not make it into 3.12, so you guys might just have to refactor for 3.13. If you really want, you can file an issue (and PR, core devs are quite busy with 3.13 imminent) for the change in cancellation. |
|
Thankfully the problem turned out to be a long standing bug in aiohttp which I just fixed in #9326 It looks like the fix in python/cpython#117407 made it more obvious as the cancellation was leaking out of the context manager because aiohttp's helper was never updated for python 3.11+ |
|
Great! Please let us know if you find any other problems ASAP so we can get them fixed before the next 3.12 patch is released on Tuesday. |
|
For history the reason we ended up using |
The one last loose end is aio-libs/aiohappyeyeballs#97 as it looks like AFAICT this is not a regression in the new code and was present before the refactor.... or my test case is bad. |
|
I'll take a closer look a little later. I'm guessing it's probably that the mock is bad, rather than a CPython problem -- |
@ZeroIntensity looks like there is a race in the new code aio-libs/aiohappyeyeballs#100 |
|
It might be worth reverting that fix before the 3.12 patch, it seems to be causing nothing but problems. |
|
@ZeroIntensity Here is a reproducer test for aio-libs/aiohappyeyeballs#100 @pytest.mark.asyncio
async def test_multiple_winners():
"""Test multiple winners are handled correctly."""
loop = asyncio.get_running_loop()
winners = []
finish = loop.create_future()
async def coro(idx):
await finish
winners.append(idx)
return idx
coros = [partial(coro, idx) for idx in range(4)]
task = loop.create_task(staggered_race(coros, delay=0.00001))
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 winner == 0
assert index == 0
assert excs == [None, None, None, None]
@pytest.mark.skipif(sys.version_info < (3, 12), reason="requires python3.12 or higher")
def test_multiple_winners_eager_task_factory():
"""Test multiple winners are handled correctly."""
loop = asyncio.new_event_loop()
eager_task_factory = asyncio.create_eager_task_factory(asyncio.Task)
loop.set_task_factory(eager_task_factory)
asyncio.set_event_loop(None)
async def run():
winners = []
finish = loop.create_future()
async def coro(idx):
await finish
winners.append(idx)
return idx
coros = [partial(coro, idx) for idx in range(4)]
task = loop.create_task(staggered_race(coros, delay=0.00001))
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 winner == 0
assert index == 0
assert excs == [None, None, None, None]
loop.run_until_complete(run()) |
|
Sorry, I haven't been able to deal with issues today (which is a bit inconvenient considering the patch is tomorrow). I think the best fix here is rolling back the fix for eager task factories and merging your implementation after you've tested it on PyPI. |
|
That sounds like a good plan. I'll followup again in a few weeks once the new implementation has had a chance to prove out in |
|
Thank you! |
|
FWIW, the revert PR for 3.12.7 is here: python/cpython#124810 This does mean that you won't get support for eager task factories on the prior versions, but that's a tradeoff we have to make here. Hopefully, if your implementation turns out to work properly, we can merge that in to 3.12 and then aiohttp will have things working. |
|
Thank you. I'll see if we can yank cc @webknjaz |
bdraco
changed the title
asyncio.staggered (fixed in cpython 3.12.7, aiohappyeyeballs 2.4.1 OR aiohappyeyeballs 2.4.2+)asyncio.staggered (fixed in aiohappyeyeballs 2.4.3+)
|
I can test it in production later this week. All my production systems are running the new implementation we shipped in aiohappyeyeballs 2.4.3 and I want to give it a bit more time to bake before switching to testing python/cpython#124847 |
|
Home Assistant is shipping the new aiohappyeyeballs today. If all goes well I'll switch over to testing that PR and do some performance comparisons as well |
|
Good to hear! |
|
The new implementation we have in aiohappyeyeballs seems to be going well. No new issues for aiohappyeyeballs filed, Home Assistant release went well, and my testing went well. I think I’m ready to start testing above PR today |
Describe the bug
discord.py 2.4.0 throws exception after upgrading aiohttp to 3.10.0:
To Reproduce
Expected behavior
no exception
Logs/tracebacks
Python Version
aiohttp Version
multidict Version
yarl Version
OS
Ubuntu 22.04
Related component
Client
Additional context
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: