Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56%. Comparing base (48c259c) to head (31ee83b).

@@          Coverage Diff          @@
##           master   #930   +/-   ##
=====================================
- Coverage      56%    56%   -1%     
=====================================
  Files          89     89           
  Lines        9756   9764    +8     
  Branches     1817   1818    +1     
=====================================
+ Hits         5472   5473    +1     
- Misses       3908   3918   +10     
+ Partials      376    373    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Yanks my previous comment here as the previous discussion has been resolved and so the comment is hidden...

This sentence: As the hubs are event loops made to dispatch I/O events and to schedule threads come from the Eventlet doc https://eventlet.readthedocs.io/en/latest/hubs.html#how-the-hubs-work

I agree with you, and with your previous analyze, we can't runs blocking things in the event loop threads, else it would block all the rest.

However, one thing related to the internal design of aiohttp bothers me.

The DNS lookups made by aiohttp and that we can observe in the stack trace in the description of #929, rely of aiohttp's ThreadedResolver.

The threaded resolver is the default used by aiodns:

• https://github.com/aio-libs/aiohttp/blob/58a9190ed092e5748e80e16082592b886d105975/aiohttp/resolver.py#L16
• https://github.com/aio-libs/aiohttp/blob/58a9190ed092e5748e80e16082592b886d105975/aiohttp/resolver.py#L118C50-L118C64

This is a by design choice because its authors think that AsyncResolver of aiodns works in 99% of cases but sometimes demonstrate different behavior in very rare conditions (aio-libs/aiohttp#2237, aio-libs/aiohttp#559): aio-libs/aiohttp#2254.

A comment in the ThreadedExecutor says:

uses an Executor for synchronous getaddrinfo() calls. concurrent.futures.ThreadPoolExecutor is used by default.

But, internally, this executor calls asyncio.loop.getaddrinfo which is an asynchronous version of socket.getaddrinfo(). See: https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.getaddrinfo. asyncio.loop.getaddrinfo is a coroutine... I won't expect it would block...

So, one thing is not clear for me. Why aiohttp should execute this coroutine in a threaded way?

Also I didn't find where the resolver is launched. I found several reference to asyncio.loop.run_in_executor (https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.run_in_executor) in aiohttp, maybe one of them launch this resolver, but if yes, as this resolver use asyncio.loop.getaddrinfo, and should not block, it is not clear to me why it should be made in a thread... .

Leading us to the bug we observed and to this PR...

In the example given in the issue at the origin of this PR, if one using the AsyncResolver it would surely have hidden this problem:

ClientSession(connector=aiohttp.TCPConnector(resolver=aiohttp.AsyncResolver()))

Fortunately the ThreadedResolver is the default one.

Coroutines never run in a thread other than the main event loop. Blocking DNS lookups (and other blocking APIs) will run in a thread pool.

To expand: concurrent.futures.thread thread pool gives you API that returns Future instances, which can be turned into asyncio-style Future instances, which can then be await-ed. But there's no coroutine involved.

Coroutines never run in a thread other than the main event loop. Blocking DNS lookups (and other blocking APIs) will run in a thread pool.

Yeah, that's my point... asyncio.loop.getaddrinfo is a coroutine (https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.loop.getaddrinfo), so why aiohttp run it in thread pool?

IMO, using the main event loop to run asyncio.loop.getaddrinfo would have been ok.