From b02143d11801d78942892cd20b975c7682da929d Mon Sep 17 00:00:00 2001 From: Jonas Obrist Date: Thu, 29 Aug 2019 16:38:39 +0900 Subject: [PATCH 1/6] fixed resolve_host race conditions --- aiohttp/connector.py | 11 ++++++----- tests/test_connector.py | 32 +++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 706bc99adb7..c1b0f6a90a6 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -778,24 +778,25 @@ async def _resolve_host(self, if (key in self._cached_hosts) and \ (not self._cached_hosts.expired(key)): + result = self._cached_hosts.next_addrs(key) if traces: for trace in traces: await trace.send_dns_cache_hit(host) - - return self._cached_hosts.next_addrs(key) + return result if key in self._throttle_dns_events: + event = self._throttle_dns_events[key] if traces: for trace in traces: await trace.send_dns_cache_hit(host) - await self._throttle_dns_events[key].wait() + await event.wait() else: + self._throttle_dns_events[key] = \ + EventResultOrError(self._loop) if traces: for trace in traces: await trace.send_dns_cache_miss(host) - self._throttle_dns_events[key] = \ - EventResultOrError(self._loop) try: if traces: diff --git a/tests/test_connector.py b/tests/test_connector.py index e40559d0c6f..6390c311f12 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -18,8 +18,9 @@ from aiohttp import client, web from aiohttp.client import ClientRequest, ClientTimeout from aiohttp.client_reqrep import ConnectionKey -from aiohttp.connector import Connection, _DNSCacheTable +from aiohttp.connector import Connection, _DNSCacheTable, TCPConnector from aiohttp.helpers import PY_37 +from aiohttp.locks import EventResultOrError from aiohttp.test_utils import make_mocked_coro, unused_port from aiohttp.tracing import Trace from conftest import needs_unix @@ -2257,3 +2258,32 @@ def test_next_addrs_single(self, dns_cache_table) -> None: addrs = dns_cache_table.next_addrs('foo') assert addrs == ['127.0.0.1'] + + +async def test_connector_cache_trace_race(): + class DummyTracer: + async def send_dns_cache_hit(self, *args, **kwargs): + connector._cached_hosts.remove(("", 0)) + + token = object() + connector = TCPConnector() + connector._cached_hosts.add(("", 0), [token]) + + traces = [DummyTracer()] + assert await connector._resolve_host("", 0, traces) == [token] + + +async def test_connector_throttle_trace_race(loop): + key = ("", 0) + token = object() + + class DummyTracer: + async def send_dns_cache_hit(self, *args, **kwargs): + event = connector._throttle_dns_events.pop(key) + event.set() + connector._cached_hosts.add(key, [token]) + + connector = TCPConnector() + connector._throttle_dns_events[key] = EventResultOrError(loop) + traces = [DummyTracer()] + assert await connector._resolve_host("", 0, traces) == [token] From e44fe1453e93586199f894cd80843a8cae79b8a4 Mon Sep 17 00:00:00 2001 From: Jonas Obrist Date: Thu, 29 Aug 2019 19:32:47 +0900 Subject: [PATCH 2/6] added missing changes file and added myself to contributors --- CHANGES/4013.bugfix | 1 + CONTRIBUTORS.txt | 1 + 2 files changed, 2 insertions(+) create mode 100644 CHANGES/4013.bugfix diff --git a/CHANGES/4013.bugfix b/CHANGES/4013.bugfix new file mode 100644 index 00000000000..1793f2137b5 --- /dev/null +++ b/CHANGES/4013.bugfix @@ -0,0 +1 @@ +Fixed race conditions in _resolve_host caching and throttling when tracing is enabled. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 21451c9ff83..f48891e83c9 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -132,6 +132,7 @@ Jian Zeng Jinkyu Yi Joel Watts Jon Nabozny +Jonas Obrist Joongi Kim Josep Cugat Joshu Coats From 0651028d8f6b83d539eddea901f0aa89ebf3b19f Mon Sep 17 00:00:00 2001 From: Jonas Obrist Date: Thu, 29 Aug 2019 21:42:37 +0900 Subject: [PATCH 3/6] fixed isort --- tests/test_connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 6390c311f12..03f5d8bd830 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -18,7 +18,7 @@ from aiohttp import client, web from aiohttp.client import ClientRequest, ClientTimeout from aiohttp.client_reqrep import ConnectionKey -from aiohttp.connector import Connection, _DNSCacheTable, TCPConnector +from aiohttp.connector import Connection, TCPConnector, _DNSCacheTable from aiohttp.helpers import PY_37 from aiohttp.locks import EventResultOrError from aiohttp.test_utils import make_mocked_coro, unused_port From d68e866febde55a65912e39d1d285638fcd707bb Mon Sep 17 00:00:00 2001 From: Jonas Obrist Date: Fri, 30 Aug 2019 19:21:43 +0900 Subject: [PATCH 4/6] Update aiohttp/connector.py Co-Authored-By: Andrew Svetlov --- aiohttp/connector.py | 1 + 1 file changed, 1 insertion(+) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index c1b0f6a90a6..a70df6ecb17 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -786,6 +786,7 @@ async def _resolve_host(self, return result if key in self._throttle_dns_events: + # get event early, before any await (#4014) event = self._throttle_dns_events[key] if traces: for trace in traces: From dd19e1e8734f92295731e23971353a66defbd3ca Mon Sep 17 00:00:00 2001 From: Jonas Obrist Date: Fri, 30 Aug 2019 19:21:50 +0900 Subject: [PATCH 5/6] Update aiohttp/connector.py Co-Authored-By: Andrew Svetlov --- aiohttp/connector.py | 1 + 1 file changed, 1 insertion(+) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index a70df6ecb17..615c5f1a21d 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -793,6 +793,7 @@ async def _resolve_host(self, await trace.send_dns_cache_hit(host) await event.wait() else: + # update dict early, before any await (#4014) self._throttle_dns_events[key] = \ EventResultOrError(self._loop) if traces: From e54d575ff8d1dda11152222f0ce1f26fc31716b5 Mon Sep 17 00:00:00 2001 From: Jonas Obrist Date: Fri, 30 Aug 2019 19:21:58 +0900 Subject: [PATCH 6/6] Update aiohttp/connector.py Co-Authored-By: Andrew Svetlov --- aiohttp/connector.py | 1 + 1 file changed, 1 insertion(+) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 615c5f1a21d..0099f467551 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -778,6 +778,7 @@ async def _resolve_host(self, 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) if traces: