Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Happy Eyeballs from Python 3.8 #4451

Closed
asvetlov opened this issue Dec 18, 2019 · 7 comments · Fixed by #7954
Closed

Support Happy Eyeballs from Python 3.8 #4451

asvetlov opened this issue Dec 18, 2019 · 7 comments · Fixed by #7954

Comments

@asvetlov
Copy link
Member

Starting from Python 3.8 asyncio supports https://tools.ietf.org/html/rfc6555

It allows working better in dual-host configurations.
It would be nice to support it by aiohttp if the feature is available by Python version.
Perhaps passing happy_eyeballs_delay and interleave to loop.create_connection() is enough.

A volunteer is welcome!

P.S.
Ooops, parameters are even not documented by asyncio docs, need to fix it.

@illia-v
Copy link
Contributor

illia-v commented Feb 26, 2021

Perhaps passing happy_eyeballs_delay and interleave to loop.create_connection() is enough.

Unfortunately, this is not enough and has no effect because host passed to loop.create_connection() is always an IP address. That happens because aiohttp translates domain names into IP addresses before calling the method.

aiohttp/aiohttp/connector.py

Lines 969 to 1025 in 8c25b44

host_resolved = asyncio.ensure_future(
self._resolve_host(host, port, traces=traces), loop=self._loop
)
try:
# Cancelling this lookup should not cancel the underlying lookup
# or else the cancel event will get broadcast to all the waiters
# across all connections.
hosts = await asyncio.shield(host_resolved)
except asyncio.CancelledError:
def drop_exception(fut: "asyncio.Future[List[Dict[str, Any]]]") -> None:
with suppress(Exception, asyncio.CancelledError):
fut.result()
host_resolved.add_done_callback(drop_exception)
raise
except OSError as exc:
# in case of proxy it is not ClientProxyConnectionError
# it is problem of resolving proxy ip itself
raise ClientConnectorError(req.connection_key, exc) from exc
last_exc = None # type: Optional[Exception]
for hinfo in hosts:
host = hinfo["host"]
port = hinfo["port"]
try:
transp, proto = await self._wrap_create_connection(
self._factory,
host,
port,
timeout=timeout,
ssl=sslcontext,
family=hinfo["family"],
proto=hinfo["proto"],
flags=hinfo["flags"],
server_hostname=hinfo["hostname"] if sslcontext else None,
local_addr=self._local_addr,
req=req,
client_error=client_error,
)
except ClientConnectorError as exc:
last_exc = exc
continue
if req.is_ssl() and fingerprint:
try:
fingerprint.check(transp)
except ServerFingerprintMismatch as exc:
transp.close()
if not self._cleanup_closed_disabled:
self._cleanup_closed_transports.append(transp)
last_exc = exc
continue
return transp, proto


An alternative is implementing the Happy Eyeballs algorithm in the aiohttp library reusing asyncio.staggered.staggered_race().

I would create a method similar to loop._connect_sock and use logic similar to this one right after determining IP addresses.

Then I'd call self._wrap_create_connection() (and subsequently loop.create_connection()) with a connected socket as sock instead of family, proto, flags and local_addr.

There would be a couple of problems though:

  • this check would be performed only once for a connection determined by Happy Eyeballs and if it failed, it would not try connecting to next IP address as it does at the moment

    aiohttp/aiohttp/connector.py

    Lines 1015 to 1023 in 8c25b44

    if req.is_ssl() and fingerprint:
    try:
    fingerprint.check(transp)
    except ServerFingerprintMismatch as exc:
    transp.close()
    if not self._cleanup_closed_disabled:
    self._cleanup_closed_transports.append(transp)
    last_exc = exc
    continue
  • sock_connect timeout that limits runtime of loop.create_connection() currently.
    async with ceil_timeout(timeout.sock_connect):
    return await self._loop.create_connection(*args, **kwargs) # type: ignore[return-value] # noqa
    The runtime would be significantly smaller because the method would use an already connected socket. Probably, we'd need to split the timeout into two separate ones.

@illia-v illia-v removed their assignment Oct 29, 2021
bdraco added a commit to home-assistant/core that referenced this issue Aug 7, 2023
fixes #97146

This problem keeps coming up over and over again and
its likely aio-libs/aiohttp#4451 will
not be solved anytime soon
@bdraco
Copy link
Member

bdraco commented Nov 6, 2023

ESPHome has the same/similar issue esphome/aioesphomeapi#233

We need a happy eyeballs implementation that supports passing in a list of resolved addresses that works for python 3.8 for aiohttp so we can solve #4451 (comment)

Ideally we build a lib for that which could be used for both esphome and aiohttp so we don't have to reinvent the wheel as I'm sure other places need it as well.

The 3.8 support will be a bit of a pain though since we need use a conditional import with https://pypi.org/project/async-stagger/ as asyncio.staggered.staggered_race is only available on newer python which makes the whole thing more complex and would make sense to make it in a new library

If I find the time to build something for esphome, I'll do it as a new library with a compatible license in case there is a chance we can reuse it here.

@bdraco
Copy link
Member

bdraco commented Nov 29, 2023

I've got three projects I'm working on that need this so I think my holiday project is going to be to build a permissive licensed library that they can all use

@bdraco
Copy link
Member

bdraco commented Nov 30, 2023

        for hinfo in hosts:
            host = hinfo["host"]
            port = hinfo["port"]

            # Strip trailing dots, certificates contain FQDN without dots.
            # See https://github.com/aio-libs/aiohttp/issues/3636
            server_hostname = (
                (req.server_hostname or hinfo["hostname"]).rstrip(".")
                if sslcontext
                else None
            )

            try:
                transp, proto = await self._wrap_create_connection(
                    self._factory,
                    host,
                    port,
                    timeout=timeout,
                    ssl=sslcontext,
                    family=hinfo["family"],
                    proto=hinfo["proto"],
                    flags=hinfo["flags"],
                    server_hostname=server_hostname,
                    local_addr=self._local_addr,
                    req=req,
                    client_error=client_error,
                )

If we swap out _wrap_create_connection with something like
self._create_happy_eyeballs_connection and still do a outer loop with all the hosts but the inner loop pops them out of the happy eyeballs connect logic once we reject one via the if req.is_ssl() and fingerprint: branch it should behave mostly the same sans the timing.

Unfortunately most of loop.create_connection needs to be reimplemented because it expects to get unresolved names and we have resolved names

Worse it looks like we have to resolve local addrs as well since technically that works now

@bdraco bdraco self-assigned this Dec 9, 2023
@bdraco
Copy link
Member

bdraco commented Dec 9, 2023

I sliced up the stdlib create_connection into a new library https://github.com/bdraco/aiohappyeyeballs to allow connecting with Happy Eyeballs when you already have a list of addrinfo and not a DNS name.

bdraco added a commit that referenced this issue Dec 9, 2023
bdraco added a commit that referenced this issue Dec 9, 2023
bdraco added a commit that referenced this issue Dec 9, 2023
@Dreamsorcerer
Copy link
Member

Is there any interest in adding something in asyncio upstream to support it as well later on?

@bdraco
Copy link
Member

bdraco commented Dec 10, 2023

I think thats a good idea to be able to land it there at some point as the code is almost 100% extracted from cpython except the utils and added typing+tests.

The current asyncio create_connection code could be broken into two functions with the new start_connection as the inner function which returns the socket, but thats getting ahead of things since it probably needs to live in the wild for a while in case we need to adjust anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants