GeezerVison

I'm marking this as DO-NOT-MERGE for the time being. aiohttp is planning on trying this sometime this week.

aiohttp is planning on trying this sometime this week

Hopefully you will test with a range of aiohttp versions -- the previous version of this PR seemed to work on some but not on others.

Yeah, that's the idea. We might not even need this PR if their new implementation turns out to work perfectly.

Don't we still need to do something to make eager tasks work? Or does that problem only occur in combination with aiohttp? Presumably there's at least one previous aiohttp version where the current main (or 3.12) branch doesn't work with aiohttp (or I don't understand the issue).

Our implementation (on main and 3.12) doesn't work with eager task factories, and most aiohttp versions (apart from the latest, IIUC, because they made their own) rely on our implementation.

Our implementation (on main and 3.12) doesn't work with eager task factories, and most aiohttp versions (apart from the latest, IIUC, because they made their own) rely on our implementation.

So we definitely have to change something right?

Yup, I'm just cautious because I don't want to break prior aiohttp versions on accident.

Thanks for the PR. I can test in production later this week

we should copy and adapt the aiohttp tests into cpython

sorry I pressed the wrong button

Feel free to copy and adapt any tests in aiohappyeyeballs and consider their relicensed as needed for cpython

I can get to porting aiohttp's pytest stuff over to us sometime later this week.

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 this PR today

I’ll start by running aiohappyeyeballs test suite against it and if it passes try to write a meaningful benchmark to compare the implementations

Ok, good to know.

I didn't test it in production yet because of the comments I posted above: It looks like there is a race where tasks can be garbage collected before they return a result:

FWIW it's a different systemic issue that we need to fix asap (and backport to 3.13)

#121264 (review)

IMO this PR shouldn't solve that specific problem and focus on a smaller change.

@@ -139,3 +139,9 @@ async def staggered_race(coro_fns, delay, *, loop=None):
         # Make sure no tasks are left running if we leave this function
         for t in running_tasks:
             t.cancel()
+            try:
+                await t
+            except (SystemExit, KeyboardInterrupt):
+                raise
+            except BaseException:
+                pass

That fixes the warning but still the aiohappyeyeballs cancellation swallow check test is failing

@@ -139,3 +139,9 @@ async def staggered_race(coro_fns, delay, *, loop=None):
         # Make sure no tasks are left running if we leave this function
         for t in running_tasks:
             t.cancel()
+            try:
+                await t
+            except (SystemExit, KeyboardInterrupt):
+                raise
+            except BaseException:
+                pass

That fixes the warning but still the aiohappyeyeballs cancellation swallow check test is failing

Yes awaiting a task and suppressing cancellation will do that. You need to use a loop and a future. But we can fix that in another PR

If aiohttp's implementation has been tested in production (and has all their tests passing), why not just copy theirs over?

@ZeroIntensity

If aiohttp's implementation has been tested in production (and has all their tests passing), why not just copy theirs over?

Can you give a link to the final diff of their fix? I can't find it in the few links to different issues I clicked through this discussion.

Here

Hm, tbqh I think the approach in this PR is more straightforward. Unless you have a specific concern I'd go with it.

@graingert feel free to go ahead and merge.

We do need to backport this though.

Hm, tbqh I think the approach in this PR is more straightforward. Unless you have a specific concern I'd go with it.

@graingert feel free to go ahead and merge.

I can't merge, I'll need a core dev to do it for me

Hm, tbqh I think the approach in this PR is more straightforward. Unless you have a specific concern I'd go with it.

I'm worried about the failing tests on aiohttp's end, but I think Thomas said that he'll address that in another PR.

I can't merge, I'll need a core dev to do it for me

Apologies, I'll go ahead and click the button then.

Thanks @graingert for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

GH-125339 is a backport of this pull request to the 3.13 branch.

GH-125340 is a backport of this pull request to the 3.12 branch.