bpo-33530: Implement Happy Eyeballs in asyncio, v2#7237

twisteroidambassador · 2018-05-30T05:59:05Z

Added two keyword arguments, delay and interleave, to
BaseEventLoop.create_connection. Happy eyeballs is activated if
delay is specified.

We now have documentation for the new arguments. staggered_race() is in its own module, but not exported to the main asyncio package.

https://bugs.python.org/issue33530

Added two keyword arguments, `delay` and `interleave`, to `BaseEventLoop.create_connection`. Happy eyeballs is activated if `delay` is specified.

1st1 · 2018-05-30T17:40:21Z

Lib/asyncio/base_events.py

+reordered.extend(_roundrobin(*addrinfos_lists))
+returnreordered
+
+


base_events.py is already overly long. Would it make sense to move these two functions to staggered.py (keeping them private)?

I think it makes sense to group all Happy Eyeballs-related functions in the same module (in that case the name staggered.py leaves something to be desired). On the other hand, if interleave is to be made meaningful without using happy eyeballs, then it feels weird to have _interleave_addrinfos somewhere else.

1st1 · 2018-05-30T17:42:54Z

Lib/asyncio/base_events.py

 local_addr=None, server_hostname=None,
-ssl_handshake_timeout=None):
+ssl_handshake_timeout=None,
+delay=None, interleave=None):


I think that delay parameter doesn't clearly convey that it switches on a completely different connect behaviour. Maybe call it "happy_eyeballs_delay"?
@asvetlov what do you think? BTW, do you guys have happy eyeballs implementation in aiohttp?

No, we have not (yet). We iterate over available hosts one by one.
BTW there is potentially related thing: SRV DNS records.
It can be useful for organizing load balancing and high available systems.
Rust supports it, would be nice to have it in asyncio too.

I prefer happy_eyeballs: HappyEyeBallsConfig=None where HappyEyeBallsConfig is a dataclass with all required configuration parameters.

I prefer happy_eyeballs: HappyEyeBallsConfig=None where HappyEyeBallsConfig is a dataclass with all required configuration parameters.
Oh, that's a bit too heavy IMO. We don't have a precedent for such API design neither in asyncio nor in any other popular APIs. And maybe interleave isn't a happy eyeballs specific thing?
I think that interleave=True is meaningful even when happy eyeballs isn't enabled:
awaitloop.create_connection(proto_factory, "google.com", 80, interleave=True) # connect to google, but shuffle the addresses that getaddrinfo resolves to
If that's the case, then we want just happy_eyeballs_delay parameter to enable happy eyeballs algorithm.

BTW there is potentially related thing: SRV DNS records.
It can be useful for organizing load balancing and high available systems.
Rust supports it, would be nice to have it in asyncio too.
I don't know how prevalent SRV is used, but since the operating system's getaddrinfo doesn't support anything but A and AAAA queries, some other DNS library will be required to do SRV queries. (Linux has libresolv, not sure whether it's cross platform.) At that point it's probably too big to fit in asyncio.

A simpler version is now folded into _interleave_addrinfos.

twisteroidambassador · 2018-06-05T10:57:23Z

Lib/asyncio/base_events.py

+iffirst_address_family_count>1:
+reordered.extend(addrinfos_lists[0][:first_address_family_count-1])
+deladdrinfos_lists[0][:first_address_family_count-1]
+reordered.extend(


I removed _roundrobin as a standalone function and folded it into _interleave_addrinfo. Do we still want to move _interleave_addrinfo to staggered.py? Now that the interleave option works independently of happy_eyeballs_delay, it feels more natural to leave _interleave_addrinfo here.

I think it's OK to keep it here now

Lib/asyncio/events.py

1st1 · 2018-06-25T19:35:04Z

@asvetlov this looks good to me now and I'm OK with merging this into 3.8. Could you please take a look too?

elprans · 2018-07-28T14:04:48Z

Doc/library/asyncio-eventloop.rst

+ in RFC 8305. A sensible default value recommended by the RFC is 0.25
+ (250 milliseconds).
+
+ * *interleave* controls address reordering when a host name resolves to


Maybe it's worth naming the parameter interleave_af to make it more explicit.

1st1 · 2018-07-28T15:20:28Z

An update on this PR:

I like it and want to have it in 3.8.
I asked @asvetlov to try it in aiohttp, as he wants to add support for HE there anyways. Basically, we're awaiting for his feedback now.

twisteroidambassador · 2018-08-02T04:01:21Z

Cool, I would love to hear feedback from real world usage. 👍

I'm also continuing to improve my Happy Eyeballs implementation in https://github.com/twisteroidambassador/async_stagger , where the goal is not strict adherence to existing asyncio behavior. I've recently added support for multiple local bind addresses, (e.g. providing both an IPv4 and an IPv6 local address, so when connecting to a remote IPv6 address the bound local address is IPv6 and vice versa). Is this feature something people will want to use? Does it make sense to add this to asyncio?

1st1 · 2018-10-01T16:17:26Z

@asvetlov I remember us discussing this PR at the sprints, and I think you told me that it's working alright in aiohttp. I think I also recall you saying that you don't like a name of some parameter or something... what was it? Let's fix and merge! :)

asvetlov · 2018-10-03T09:11:15Z

The name is Lib/asyncio/staggered.py

I still didn't backport the PR into aiohttp but can do it (and test on real users :D ) later

Lib/asyncio/__init__.py

Do not expose .staggered submodule in __all__. Co-Authored-By: twisteroidambassador <twisteroidambassador@users.noreply.github.com>

miss-islington · 2019-05-05T11:14:36Z

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

asvetlov · 2019-05-05T11:15:28Z

@twisteroidambassador thank you for your patience and responsibility to fix comments!

* master: (1204 commits) bpo-31855: unittest.mock.mock_open() results now respects the argument of read([size]) (pythonGH-11521) Forbid creating of stream objects outside of asyncio (python#13101) bpo-35925: Skip SSL tests that fail due to weak external certs. (pythonGH-13124) Fix rst formatting for several links in ssl documentation (pythonGH-13133) bpo-36542: Allow to overwrite the signature for Python functions. (pythonGH-12705) bpo-36793: Remove unneeded __str__ definitions. (pythonGH-13081) bpo-36766: Typos in docs and code comments (pythonGH-13116) bpo-36275: enhance documentation for venv.create() (pythonGH-13114) Clarify the download unit in the download section (pythonGH-13122) bpo-30668: add missing word in license.rst (pythonGH-13115) Unroll import-team in CODEOWNERS (python#13118) bpo-36594: Fix incorrect use of %p in format strings (pythonGH-12769) bpo-36798: Updating f-string docs for := use case (pythonGH-13107) Update wsgiref.rst (python#10488) Doc/c-api/exceptions.rst: fix grammar (python#12091) bpo-36811: Fix a C compiler warning in _elementtree.c. (pythonGH-13109) Only count number of members once (python#12691) bpo-16024: Doc cleanup regarding path_fd, dir_fd, follow_symlinks (pythonGH-5505) bpo-36791: Safer detection of integer overflow in sum(). (pythonGH-13080) bpo-33530: Implement Happy Eyeballs in asyncio, v2 (pythonGH-7237) ...

twisteroidambassador added 13 commits May 24, 2018 20:14

Implement Happy Eyeballs in asyncio.
a755bbb
Added two keyword arguments, `delay` and `interleave`, to `BaseEventLoop.create_connection`. Happy eyeballs is activated if `delay` is specified.

Use module import instead of individual imports.
fc29450

Change TODO.
d792c43

Rename helpers.py to staggered.py.
f9111d0

Add create_connection()'s new arguments in documentation.
ded34e0

Add blurb.
b069c95

Implement Happy Eyeballs in asyncio.
c5d3a92
Added two keyword arguments, `delay` and `interleave`, to `BaseEventLoop.create_connection`. Happy eyeballs is activated if `delay` is specified.

Use module import instead of individual imports.
38c7caa

Change TODO.
70ec96d

Rename helpers.py to staggered.py.
cef0a76

Add create_connection()'s new arguments in documentation.
73a4a5a

Add blurb.
b3a6e1c

Merge remote-tracking branch 'origin/happy-eyeballs' into happy-eyeballs
632166d

twisteroidambassador requested review from 1st1 and asvetlov as code owners May 30, 2018 05:59

the-knights-who-say-ni added the CLA signed label May 30, 2018

bedevere-bot added the awaiting review label May 30, 2018

1st1 reviewed May 30, 2018
View reviewed changes

twisteroidambassador added 5 commits June 3, 2018 15:23

Remove _roundrobin as a standalone function.
b8d7e41
A simpler version is now folded into _interleave_addrinfos.

Rename delay to happy_eyeballs_delay and decouple from interleave.
ecdc83a

Update docs.
5fa5a9b

Update comments.
321e4ac

Import staggered_race into main asyncio package.
b4227ed

twisteroidambassador commented Jun 5, 2018
View reviewed changes

1st1 reviewed Jun 25, 2018
View reviewed changes

Lib/asyncio/events.py Outdated Show resolvedHide resolved

twisteroidambassador added 3 commits June 26, 2018 10:16

Change delay to happy_eyeballs_delay in AbstractEventLoop.
cffacc7

Change delay to happy_eyeballs_delay in documentation and blurb.
3e304dd

Add suggested value for happy_eyeballs_delay in documentation.
a8c39b9

elprans reviewed Jul 28, 2018
View reviewed changes

Tune RST markup
f6f2219

asvetlov reviewed May 3, 2019
View reviewed changes

Lib/asyncio/__init__.py Outdated Show resolvedHide resolved
Lib/asyncio/__init__.py Outdated Show resolvedHide resolved

Keep .staggered as private API
f0e37e6
Do not expose .staggered submodule in __all__. Co-Authored-By: twisteroidambassador <twisteroidambassador@users.noreply.github.com>

asvetlov approved these changes May 5, 2019
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels May 5, 2019

asvetlov added 🤖 automerge topic-asyncio labels May 5, 2019

miss-islington merged commit 88f07a8 into python:masterMay 5, 2019

bedevere-bot removed the awaiting merge label May 5, 2019

Jc2k mentioned this pull request Jan 28, 2023
IPV6 not failing back to IPV4 home-assistant/analytics.home-assistant.io#684
Closed

		reordered.extend(_roundrobin(*addrinfos_lists))
		returnreordered

Uh oh!

bpo-33530: Implement Happy Eyeballs in asyncio, v2#7237

bpo-33530: Implement Happy Eyeballs in asyncio, v2 #7237

Uh oh!

Conversation

twisteroidambassador commented May 30, 2018• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1st1 commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Jul 28, 2018

Uh oh!

twisteroidambassador commented Aug 2, 2018

Uh oh!

1st1 commented Oct 1, 2018

Uh oh!

asvetlov commented Oct 3, 2018

Uh oh!

Uh oh!

Uh oh!

miss-islington commented May 5, 2019

Uh oh!

asvetlov commented May 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

twisteroidambassador commented May 30, 2018•
edited by bedevere-bot
Loading