search

aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python
https://docs.aiohttp.org
Other
14.97k stars 2k forks source link

AsyncResolver not passing on loop too aiodns.DNSResolver #8958

Closed mvdwetering closed 2 weeks ago

mvdwetering commented 2 weeks ago

Describe the bug

I am trying to use a TCPConnector with loop parameter. This resulted in this error. 

  File "/home/michel/hass_dev/venv/lib/python3.12/site-packages/aiohttp/connector.py", line 817, in __init__
    resolver = DefaultResolver(loop=self._loop)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/michel/hass_dev/venv/lib/python3.12/site-packages/aiohttp/resolver.py", line 94, in __init__
    self._resolver = aiodns.DNSResolver(*args, **kwargs)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/michel/hass_dev/venv/lib/python3.12/site-packages/aiodns/__init__.py", line 53, in __init__
    self.loop = loop or asyncio.get_event_loop()
                        ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/asyncio/events.py", line 702, in get_event_loop
    raise RuntimeError('There is no current event loop in thread %r.'

So aiodns can't get current loop, but that should have been passed in.

Looking through the code it seems that the loop parameter is not passed on when creating the aiodns.DNSResolver here

    def __init__(
        self,
        loop: Optional[asyncio.AbstractEventLoop] = None,
        *args: Any,
        **kwargs: Any
    ) -> None:
        if aiodns is None:
            raise RuntimeError("Resolver requires aiodns library")

        self._resolver = aiodns.DNSResolver(*args, **kwargs)

I could not figure out how to build/install aiohttp from source, but by manually changing the file in the site-packages dir in my venv to pass on the loop like below it works.

        self._resolver = aiodns.DNSResolver(loop=loop, *args, **kwargs)

To Reproduce

I am not entirely sure.

I think it is because I provide a TCPConnector to the session in which I provide the loop parameter which later on fails with the DNS lookup.

I tried to modify the sample in the readme for a minimal reproduction example, but could not get it to trigger :(

Also the issue does not trigger in my library example, it does trigger when the library is used from Home Assistant, not sure why.

This is the code that builds the clientsession 

    async def _get_clientsession(self) -> aiohttp.ClientSession:
        """
        Get a clientsession that is tuned for communication with the Hue Syncbox
        """

        def _get_aiohttp_client_session(loop) -> aiohttp.ClientSession:
            context = ssl.create_default_context(cadata=HSB_CACERT)
            context.hostname_checks_common_name = True

            connector = aiohttp.TCPConnector(
                enable_cleanup_closed=True,  # Home Assistant sets it so lets do it also
                ssl=context,
                limit_per_host=1,  # Syncbox can handle a limited amount of connections, only take what we need
                loop=loop  # Need to provide loop manually because running in executor
            )

            return aiohttp.ClientSession(
                connector=connector, timeout=aiohttp.ClientTimeout(total=10)
            )

        # Creating a session has some blocking IO code so we need to run it in the executor
        loop = asyncio.get_running_loop()
        session = await loop.run_in_executor(None, _get_aiohttp_client_session, loop)

        return session

Expected behavior

Can provide loop to TCPCOnnector and gets used.

Logs/tracebacks

Already put in description

Python Version

$ python --version
Python 3.12.4

aiohttp Version

$ python -m pip show aiohttp
Name: aiohttp
Version: 3.10.5
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache 2
Location: /home/michel/hass_dev/venv/lib/python3.12/site-packages
Requires: aiohappyeyeballs, aiosignal, attrs, frozenlist, multidict, yarl
Required-by: aiohttp-cors, aiohttp-fast-url-dispatcher, aiohttp-fast-zlib, aiohttp-isal, aiohttp-session, aiohue, aiohuesyncbox, aioytmdesktopapi, async_upnp_client, hass-nabucasa, homeassistant, pytest-aiohttp, python-matter-server, snitun

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.0.5
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /home/michel/hass_dev/venv/lib/python3.12/site-packages
Requires: 
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.9.4
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache-2.0
Location: /home/michel/hass_dev/venv/lib/python3.12/site-packages
Requires: idna, multidict
Required-by: aiohttp, homeassistant

OS

Ubuntu 22.04 in WSL

Related component

Client

Additional context

No response

Code of Conduct

Dreamsorcerer commented 2 weeks ago

The loop parameter is deprecated in nearly everywhere in asyncio, and doesn't look like it exists on TCPConnector in master, so I'd avoid doing this. At a glance, I also don't see any blocking I/O in ClientSession that the comment refers to, I think it might just be the SSLContext that might be a little slow to create.

@bdraco Are you seeing things like this in homeassistant?

mvdwetering commented 2 weeks ago

Ah, I did not know about loop parameter being deprecated.

I got to this state due to a "Detected blocking call" warning from Home Assistant 2024.9.0rc after which I started wrapping things into an executor (without knowing too much about them) which gave me an error that the BaseConnector could not figure out current loop, which lead to me adding loop parameter which in turn lead to aiodns complaining about the same not being able to find current loop. Which made me end up here

Looking better at the warning from Home Assistant it indeed shows the SSL context.load_verify_locations as offender. I initially just looked at the stacktrace and started wrapping all in the last function into an exectuor. So too much in make-errors-go-away-mode and not enough thinking :/

I just now tried wrapping only the SSL context in an executor and that works (and does not need a loop) So my initial problem is solved (sorry for getting off-topic a bit)

But I guess not passing on the loop parameter to aiodns.DNSResolve is technically still an issue?

bdraco commented 2 weeks ago

Its only load_verify_locations https://developers.home-assistant.io/docs/asyncio_blocking_operations#load_verify_locations that needs to be run in the executor. Most aiohttp objects should be created in the event loop thread.

But I guess not passing on the loop parameter to aiodns.DNSResolve is technically still an issue?

When the loop param going away in most places, it likely something we wouldn't want to add support for.

Dreamsorcerer commented 2 weeks ago

Even if aiodns has a loop parameter, I'd expect them to remove it at some point as we have done, and as they are on a different release schedule to us, that'd just end up breaking it again. So, probably not much point in us passing the loop through.