From 313ddea92aa8171b3c84c01a601ba26e13ae1644 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 18 Feb 2024 12:48:16 -0600 Subject: [PATCH 01/15] Avoid creating a task to do dns resolution if there is no throttle --- aiohttp/connector.py | 50 +++++++++++++++++++++++++++++------------ tests/test_connector.py | 6 +++++ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 47c32cd1f3e..1211d774c06 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -814,6 +814,7 @@ def clear_dns_cache( async def _resolve_host( self, host: str, port: int, traces: Optional[List["Trace"]] = None ) -> List[Dict[str, Any]]: + """Resolves host and returns list of addresses.""" if is_ip_address(host): return [ { @@ -840,8 +841,7 @@ async def _resolve_host( return res key = (host, port) - - if (key in self._cached_hosts) and (not self._cached_hosts.expired(key)): + if key in self._cached_hosts and not self._cached_hosts.expired(key): # get result early, before any await (#4014) result = self._cached_hosts.next_addrs(key) @@ -850,6 +850,39 @@ async def _resolve_host( await trace.send_dns_cache_hit(host) return result + # + # If multiple connectors are resolving the same host, we wait + # for the first one to resolve and then use the result for all of them. + # We use a throttle event to ensure that we only resolve the host once + # and then use the result for all the waiters. + # + # In this case we need to create a task to ensure that we can shield + # the task from cancellation as cancelling this lookup should not cancel + # the underlying lookup or else the cancel event will get broadcast to + # all the waiters across all connections. + # + host_resolved = asyncio.create_task( + self._resolve_host_with_throttle(key, host, port, traces) + ) + try: + return 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 + + async def _resolve_host_with_throttle( + self, + key: Tuple[str, int], + host: str, + port: int, + traces: Optional[List["Trace"]], + ) -> List[Dict[str, Any]]: + """Resolve host with a dns events throttle.""" if key in self._throttle_dns_events: # get event early, before any await (#4014) event = self._throttle_dns_events[key] @@ -1136,22 +1169,11 @@ async def _create_direct_connection( host = host.rstrip(".") + "." port = req.port assert port is not None - 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 + hosts = await self._resolve_host(host, port, traces=traces) except OSError as exc: if exc.errno is None and isinstance(exc, asyncio.TimeoutError): raise diff --git a/tests/test_connector.py b/tests/test_connector.py index a7b92ebbd21..00faeaf44d1 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1021,6 +1021,7 @@ async def test_tcp_connector_dns_throttle_requests( loop.create_task(conn._resolve_host("localhost", 8080)) loop.create_task(conn._resolve_host("localhost", 8080)) await asyncio.sleep(0) + await asyncio.sleep(0) m_resolver().resolve.assert_called_once_with("localhost", 8080, family=0) @@ -1032,6 +1033,9 @@ async def test_tcp_connector_dns_throttle_requests_exception_spread(loop: Any) - r1 = loop.create_task(conn._resolve_host("localhost", 8080)) r2 = loop.create_task(conn._resolve_host("localhost", 8080)) await asyncio.sleep(0) + await asyncio.sleep(0) + await asyncio.sleep(0) + await asyncio.sleep(0) assert r1.exception() == e assert r2.exception() == e @@ -1045,6 +1049,7 @@ async def test_tcp_connector_dns_throttle_requests_cancelled_when_close( loop.create_task(conn._resolve_host("localhost", 8080)) f = loop.create_task(conn._resolve_host("localhost", 8080)) + await asyncio.sleep(0) await asyncio.sleep(0) await conn.close() @@ -1212,6 +1217,7 @@ async def test_tcp_connector_dns_tracing_throttle_requests( loop.create_task(conn._resolve_host("localhost", 8080, traces=traces)) loop.create_task(conn._resolve_host("localhost", 8080, traces=traces)) await asyncio.sleep(0) + await asyncio.sleep(0) on_dns_cache_hit.assert_called_once_with( session, trace_config_ctx, aiohttp.TraceDnsCacheHitParams("localhost") ) From 1c0af2f7479b32561f0db4dd6ab32f3caf07a05e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:19:07 -0600 Subject: [PATCH 02/15] naming resolved_host_task --- aiohttp/connector.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 1211d774c06..1bbc71dc38d 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -861,18 +861,18 @@ async def _resolve_host( # the underlying lookup or else the cancel event will get broadcast to # all the waiters across all connections. # - host_resolved = asyncio.create_task( + resolved_host_task = asyncio.create_task( self._resolve_host_with_throttle(key, host, port, traces) ) try: - return await asyncio.shield(host_resolved) + return await asyncio.shield(resolved_host_task) except asyncio.CancelledError: - def drop_exception(fut: "asyncio.Future[List[Dict[str, Any]]]") -> None: + 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) + resolved_host_task.add_done_callback(drop_exception) raise async def _resolve_host_with_throttle( From 2367798d26f4d8928e618976f8d231e2160ce16b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:19:50 -0600 Subject: [PATCH 03/15] pep257 --- aiohttp/connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 1bbc71dc38d..16aeb97a719 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -814,7 +814,7 @@ def clear_dns_cache( async def _resolve_host( self, host: str, port: int, traces: Optional[List["Trace"]] = None ) -> List[Dict[str, Any]]: - """Resolves host and returns list of addresses.""" + """Resolve host and returns list of addresses.""" if is_ip_address(host): return [ { From 8609c35605022650ca07d04fdedd76f708b7b372 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:23:16 -0600 Subject: [PATCH 04/15] remove some duplicate sleeps --- tests/test_connector.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 00faeaf44d1..81f07e2d68d 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1020,8 +1020,7 @@ async def test_tcp_connector_dns_throttle_requests( m_resolver().resolve.return_value = dns_response() loop.create_task(conn._resolve_host("localhost", 8080)) loop.create_task(conn._resolve_host("localhost", 8080)) - await asyncio.sleep(0) - await asyncio.sleep(0) + await asyncio.sleep(0.01) m_resolver().resolve.assert_called_once_with("localhost", 8080, family=0) @@ -1032,10 +1031,7 @@ async def test_tcp_connector_dns_throttle_requests_exception_spread(loop: Any) - m_resolver().resolve.side_effect = e r1 = loop.create_task(conn._resolve_host("localhost", 8080)) r2 = loop.create_task(conn._resolve_host("localhost", 8080)) - await asyncio.sleep(0) - await asyncio.sleep(0) - await asyncio.sleep(0) - await asyncio.sleep(0) + await asyncio.sleep(0.01) assert r1.exception() == e assert r2.exception() == e From 2151a80f05d0148a10406234762e07e17c4c2341 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:24:47 -0600 Subject: [PATCH 05/15] remove some duplicate sleeps --- tests/test_connector.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 81f07e2d68d..e1f23f15c26 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1212,8 +1212,7 @@ async def test_tcp_connector_dns_tracing_throttle_requests( m_resolver().resolve.return_value = dns_response() loop.create_task(conn._resolve_host("localhost", 8080, traces=traces)) loop.create_task(conn._resolve_host("localhost", 8080, traces=traces)) - await asyncio.sleep(0) - await asyncio.sleep(0) + await asyncio.sleep(0.1) on_dns_cache_hit.assert_called_once_with( session, trace_config_ctx, aiohttp.TraceDnsCacheHitParams("localhost") ) From b3616468785cb4456482162d0d7795192908528f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:26:51 -0600 Subject: [PATCH 06/15] timeline --- CHANGES/8163.misc.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/8163.misc.rst diff --git a/CHANGES/8163.misc.rst b/CHANGES/8163.misc.rst new file mode 100644 index 00000000000..57e986ee193 --- /dev/null +++ b/CHANGES/8163.misc.rst @@ -0,0 +1 @@ +Avoid creating a task to do dns resolution if there is no throttle From 7a037050e891e9f1d91e256df9a2d7e2f6655fcb Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:27:19 -0600 Subject: [PATCH 07/15] DNS --- CHANGES/8163.misc.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/8163.misc.rst b/CHANGES/8163.misc.rst index 57e986ee193..ae41601cd91 100644 --- a/CHANGES/8163.misc.rst +++ b/CHANGES/8163.misc.rst @@ -1 +1 @@ -Avoid creating a task to do dns resolution if there is no throttle +Avoid creating a task to do DNS resolution if there is no throttle From 38e0649023c32bcb2c986a0aa744f5ad17799c32 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 08:33:56 -0600 Subject: [PATCH 08/15] re-quote since TypeError: 'type' object is not subscriptable on py3.8 --- aiohttp/connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 16aeb97a719..42ea11eff46 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -868,7 +868,7 @@ async def _resolve_host( return await asyncio.shield(resolved_host_task) except asyncio.CancelledError: - def drop_exception(fut: asyncio.Future[List[Dict[str, Any]]]) -> None: + def drop_exception(fut: "asyncio.Future[List[Dict[str, Any]]]") -> None: with suppress(Exception, asyncio.CancelledError): fut.result() From 70ec445198babd96e1990b948286de01b045cea0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 09:03:18 -0600 Subject: [PATCH 09/15] Revert "remove some duplicate sleeps" This reverts commit 2151a80f05d0148a10406234762e07e17c4c2341. --- tests/test_connector.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index e1f23f15c26..81f07e2d68d 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1212,7 +1212,8 @@ async def test_tcp_connector_dns_tracing_throttle_requests( m_resolver().resolve.return_value = dns_response() loop.create_task(conn._resolve_host("localhost", 8080, traces=traces)) loop.create_task(conn._resolve_host("localhost", 8080, traces=traces)) - await asyncio.sleep(0.1) + await asyncio.sleep(0) + await asyncio.sleep(0) on_dns_cache_hit.assert_called_once_with( session, trace_config_ctx, aiohttp.TraceDnsCacheHitParams("localhost") ) From f53bbf3825e296b071080fd047f89c1807257571 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 09:03:19 -0600 Subject: [PATCH 10/15] Revert "remove some duplicate sleeps" This reverts commit 8609c35605022650ca07d04fdedd76f708b7b372. --- tests/test_connector.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 81f07e2d68d..00faeaf44d1 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1020,7 +1020,8 @@ async def test_tcp_connector_dns_throttle_requests( m_resolver().resolve.return_value = dns_response() loop.create_task(conn._resolve_host("localhost", 8080)) loop.create_task(conn._resolve_host("localhost", 8080)) - await asyncio.sleep(0.01) + await asyncio.sleep(0) + await asyncio.sleep(0) m_resolver().resolve.assert_called_once_with("localhost", 8080, family=0) @@ -1031,7 +1032,10 @@ async def test_tcp_connector_dns_throttle_requests_exception_spread(loop: Any) - m_resolver().resolve.side_effect = e r1 = loop.create_task(conn._resolve_host("localhost", 8080)) r2 = loop.create_task(conn._resolve_host("localhost", 8080)) - await asyncio.sleep(0.01) + await asyncio.sleep(0) + await asyncio.sleep(0) + await asyncio.sleep(0) + await asyncio.sleep(0) assert r1.exception() == e assert r2.exception() == e From b1ce15a3136f0d55f673989fd95eb895c309d6f7 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 10:01:44 -0600 Subject: [PATCH 11/15] Update aiohttp/connector.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) --- aiohttp/connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 42ea11eff46..2dea12d7adf 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -814,7 +814,7 @@ def clear_dns_cache( async def _resolve_host( self, host: str, port: int, traces: Optional[List["Trace"]] = None ) -> List[Dict[str, Any]]: - """Resolve host and returns list of addresses.""" + """Resolve host and return list of addresses.""" if is_ip_address(host): return [ { From 750b5cc61d1cb0170b551d289532627471624a4c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 10:02:21 -0600 Subject: [PATCH 12/15] declare bugfix --- CHANGES/{8163.misc.rst => 8163.bugfix.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename CHANGES/{8163.misc.rst => 8163.bugfix.rst} (100%) diff --git a/CHANGES/8163.misc.rst b/CHANGES/8163.bugfix.rst similarity index 100% rename from CHANGES/8163.misc.rst rename to CHANGES/8163.bugfix.rst From d68641885fcf65093eb2ea0c5c5bdace4c33fb24 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 10:45:47 -0600 Subject: [PATCH 13/15] Update CHANGES/8163.bugfix.rst MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) --- CHANGES/8163.bugfix.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGES/8163.bugfix.rst b/CHANGES/8163.bugfix.rst index ae41601cd91..c2ae312129b 100644 --- a/CHANGES/8163.bugfix.rst +++ b/CHANGES/8163.bugfix.rst @@ -1 +1,5 @@ -Avoid creating a task to do DNS resolution if there is no throttle +Improved the not-throttled DNS resolution performance +-- by :user:`bdraco`. + +This is achieverd by avoiding an :mod:`asyncio` task creation +in this case. From a5df7522a920fd29c488ae801c12b869a0c49535 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 10:46:02 -0600 Subject: [PATCH 14/15] Update CHANGES/8163.bugfix.rst --- CHANGES/8163.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/8163.bugfix.rst b/CHANGES/8163.bugfix.rst index c2ae312129b..8ac632a3d7e 100644 --- a/CHANGES/8163.bugfix.rst +++ b/CHANGES/8163.bugfix.rst @@ -1,5 +1,5 @@ Improved the not-throttled DNS resolution performance -- by :user:`bdraco`. -This is achieverd by avoiding an :mod:`asyncio` task creation +This is achieved by avoiding an :mod:`asyncio` task creation in this case. From 09fa352cbf10e3b9ef214fb6415e6197fa14242d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 19 Feb 2024 11:05:08 -0600 Subject: [PATCH 15/15] Update CHANGES/8163.bugfix.rst --- CHANGES/8163.bugfix.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/8163.bugfix.rst b/CHANGES/8163.bugfix.rst index 8ac632a3d7e..8bfb10260c6 100644 --- a/CHANGES/8163.bugfix.rst +++ b/CHANGES/8163.bugfix.rst @@ -1,4 +1,4 @@ -Improved the not-throttled DNS resolution performance +Improved the DNS resolution performance on cache hit -- by :user:`bdraco`. This is achieved by avoiding an :mod:`asyncio` task creation