From b9d776df4848565f38c5ebe42d49030b3194a1a0 Mon Sep 17 00:00:00 2001 From: Brett Higgins Date: Mon, 17 Jul 2023 10:50:57 -0400 Subject: [PATCH 01/12] Fixed failure to try next host after single-host connection timeout Also updated the default ``ClientTimeout`` params to include ``sock_connect`` so that this correct behavior happens by default. --- CHANGES/7342.bugfix | 3 ++ CONTRIBUTORS.txt | 1 + aiohttp/client.py | 2 +- aiohttp/connector.py | 2 +- docs/client_quickstart.rst | 3 +- docs/client_reference.rst | 4 +- tests/test_connector.py | 75 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 CHANGES/7342.bugfix diff --git a/CHANGES/7342.bugfix b/CHANGES/7342.bugfix new file mode 100644 index 00000000000..fa411657e0c --- /dev/null +++ b/CHANGES/7342.bugfix @@ -0,0 +1,3 @@ +Fixed failure to try next host after single-host connection timeout. +Also updated the default ``ClientTimeout`` params to include ``sock_connect`` +so that this correct behavior happens by default. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 0cb642bf466..25bf3ee7792 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -60,6 +60,7 @@ Boris Feld Borys Vorona Boyi Chen Brett Cannon +Brett Higgins Brian Bouterse Brian C. Lane Brian Muller diff --git a/aiohttp/client.py b/aiohttp/client.py index 9050cc4120c..777722e2c27 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -157,7 +157,7 @@ class ClientTimeout: # 5 Minute default read timeout -DEFAULT_TIMEOUT: Final[ClientTimeout] = ClientTimeout(total=5 * 60) +DEFAULT_TIMEOUT: Final[ClientTimeout] = ClientTimeout(total=5 * 60, sock_connect=30) _RetType = TypeVar("_RetType") diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 3d9f7b59f7b..dcaeb72b6a5 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1131,7 +1131,7 @@ def drop_exception(fut: "asyncio.Future[List[Dict[str, Any]]]") -> None: req=req, client_error=client_error, ) - except ClientConnectorError as exc: + except (ClientConnectorError, asyncio.TimeoutError) as exc: last_exc = exc continue diff --git a/docs/client_quickstart.rst b/docs/client_quickstart.rst index 92334a5f4b4..cb5a4f2ce3f 100644 --- a/docs/client_quickstart.rst +++ b/docs/client_quickstart.rst @@ -407,7 +407,8 @@ Timeouts Timeout settings are stored in :class:`ClientTimeout` data structure. By default *aiohttp* uses a *total* 300 seconds (5min) timeout, it means that the -whole operation should finish in 5 minutes. +whole operation should finish in 5 minutes. In order to allow time for DNS fallback, +the default ``sock_connect`` timeout is 30 seconds. The value could be overridden by *timeout* parameter for the session (specified in seconds):: diff --git a/docs/client_reference.rst b/docs/client_reference.rst index 407030dce14..464cc06c2c8 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -135,7 +135,7 @@ The client session supports the context manager protocol for self closing. higher. :param timeout: a :class:`ClientTimeout` settings structure, 300 seconds (5min) - total timeout by default. + total timeout, 30 seconds socket connect timeout by default. .. versionadded:: 3.3 @@ -905,7 +905,7 @@ certification chaining. .. versionadded:: 3.7 :param timeout: a :class:`ClientTimeout` settings structure, 300 seconds (5min) - total timeout by default. + total timeout, 30 seconds socket connect timeout by default. :param loop: :ref:`event loop` used for processing HTTP requests. diff --git a/tests/test_connector.py b/tests/test_connector.py index 9ef71882f87..8a344a82953 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -669,6 +669,81 @@ def get_extra_info(param): established_connection.close() +async def test_tcp_connector_multiple_hosts_one_timeout(loop: Any) -> None: + conn = aiohttp.TCPConnector() + + ip1 = "192.168.1.1" + ip2 = "192.168.1.2" + ips = [ip1, ip2] + ips_tried = [] + + req = ClientRequest( + "GET", + URL("https://mocked.host"), + loop=loop, + ) + + async def _resolve_host(host, port, traces=None): + return [ + { + "hostname": host, + "host": ip, + "port": port, + "family": socket.AF_INET, + "proto": 0, + "flags": socket.AI_NUMERICHOST, + } + for ip in ips + ] + + conn._resolve_host = _resolve_host + + timeout_error = False + connected = False + + async def create_connection(*args, **kwargs): + nonlocal timeout_error, connected + + ip = args[1] + + ips_tried.append(ip) + + if ip == ip1: + timeout_error = True + raise asyncio.TimeoutError + + if ip == ip2: + connected = True + tr = create_mocked_conn(loop) + pr = create_mocked_conn(loop) + + def get_extra_info(param): + if param == "sslcontext": + return True + + if param == "ssl_object": + s = create_mocked_conn(loop) + s.getpeercert.return_value = b"foo" + return s + + assert False + + tr.get_extra_info = get_extra_info + return tr, pr + + assert False + + conn._loop.create_connection = create_connection + + established_connection = await conn.connect(req, [], ClientTimeout()) + assert ips == ips_tried + + assert timeout_error + assert connected + + established_connection.close() + + async def test_tcp_connector_resolve_host(loop: Any) -> None: conn = aiohttp.TCPConnector(use_dns_cache=True) From e32f7090a4955a7c6be1ffa71be046ba3b3be0e7 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 11 Sep 2024 16:18:58 +0000 Subject: [PATCH 02/12] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_connector.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 34ec160ebd6..75a9780a43a 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1067,7 +1067,9 @@ async def create_connection( established_connection.close() -async def test_tcp_connector_multiple_hosts_one_timeout(loop: asyncio.AbstractEventLoop) -> None: +async def test_tcp_connector_multiple_hosts_one_timeout( + loop: asyncio.AbstractEventLoop, +) -> None: conn = aiohttp.TCPConnector() ip1 = "192.168.1.1" @@ -1081,7 +1083,9 @@ async def test_tcp_connector_multiple_hosts_one_timeout(loop: asyncio.AbstractEv loop=loop, ) - async def _resolve_host(host: str, por: intt, traces: Optional[Sequence[Trace]] = None) -> List[ResolveResult]: + async def _resolve_host( + host: str, por: intt, traces: Optional[Sequence[Trace]] = None + ) -> List[ResolveResult]: return [ { "hostname": host, @@ -1099,7 +1103,9 @@ async def _resolve_host(host: str, por: intt, traces: Optional[Sequence[Trace]] timeout_error = False connected = False - async def create_connection(*args: object, **kwargs: object) -> Tuple[mock.Mock, mock.Mock]: + async def create_connection( + *args: object, **kwargs: object + ) -> Tuple[mock.Mock, mock.Mock]: nonlocal timeout_error, connected ip = args[1] From 7e408c8421819afa68931821c64c7152591a147b Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Wed, 11 Sep 2024 17:24:11 +0100 Subject: [PATCH 03/12] Update test_connector.py --- tests/test_connector.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 75a9780a43a..b7f588986ba 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1084,7 +1084,7 @@ async def test_tcp_connector_multiple_hosts_one_timeout( ) async def _resolve_host( - host: str, por: intt, traces: Optional[Sequence[Trace]] = None + host: str, port: int, traces: Optional[Sequence[Trace]] = None ) -> List[ResolveResult]: return [ { @@ -1098,8 +1098,6 @@ async def _resolve_host( for ip in ips ] - conn._resolve_host = _resolve_host - timeout_error = False connected = False @@ -1121,7 +1119,7 @@ async def create_connection( tr = create_mocked_conn(loop) pr = create_mocked_conn(loop) - def get_extra_info(param): + def get_extra_info(param: str) -> Union[bool, mock.Mock]: if param == "sslcontext": return True @@ -1137,15 +1135,15 @@ def get_extra_info(param): assert False - conn._loop.create_connection = create_connection - - established_connection = await conn.connect(req, [], ClientTimeout()) - assert ips == ips_tried - - assert timeout_error - assert connected - - established_connection.close() + with mock.patch.object(conn, "_resolve_host", _resolve_host): + with mock.patch.object(conn._loop, "create_connection", create_connection): + established_connection = await conn.connect(req, [], ClientTimeout()) + assert ips == ips_tried + + assert timeout_error + assert connected + + established_connection.close() async def test_tcp_connector_resolve_host(loop: asyncio.AbstractEventLoop) -> None: From 5bb72ff955308c1fa29154609da65500959fc19c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 11 Sep 2024 16:25:03 +0000 Subject: [PATCH 04/12] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_connector.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index b7f588986ba..6fa16560eee 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1139,10 +1139,10 @@ def get_extra_info(param: str) -> Union[bool, mock.Mock]: with mock.patch.object(conn._loop, "create_connection", create_connection): established_connection = await conn.connect(req, [], ClientTimeout()) assert ips == ips_tried - + assert timeout_error assert connected - + established_connection.close() From 40fe8551a977a6ff3af8851d418557832607de38 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Wed, 11 Sep 2024 17:30:08 +0100 Subject: [PATCH 05/12] Update test_connector.py --- tests/test_connector.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_connector.py b/tests/test_connector.py index 6fa16560eee..5f64003a116 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -21,6 +21,7 @@ Optional, Sequence, Tuple, + Union, ) from unittest import mock From 6756b4a2cd626f241ad6d05deef1ff39a31dd71e Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Mon, 30 Sep 2024 00:11:48 +0100 Subject: [PATCH 06/12] Update test_connector.py --- tests/test_connector.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_connector.py b/tests/test_connector.py index 5f64003a116..7dfaed50be1 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1068,7 +1068,13 @@ async def create_connection( established_connection.close() +@mock.patch( + "aiohttp.connector.aiohappyeyeballs.start_connection", + autospec=True, + spec_set=True, +) async def test_tcp_connector_multiple_hosts_one_timeout( + start_connection: mock.Mock, loop: asyncio.AbstractEventLoop, ) -> None: conn = aiohttp.TCPConnector() From 7a050384b3c4aa5e9aa5161075f711e8203148f8 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 1 Oct 2024 11:46:57 -0500 Subject: [PATCH 07/12] update tests --- tests/test_connector.py | 95 +++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 33 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 63dff162f47..3293c6bfc83 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -21,7 +21,6 @@ Optional, Sequence, Tuple, - Union, ) from unittest import mock @@ -1064,6 +1063,13 @@ async def create_connection( established_connection.close() +@pytest.mark.parametrize( + ("request_url"), + [ + ("http://mocked.host"), + ("https://mocked.host"), + ], +) @mock.patch( "aiohttp.connector.aiohappyeyeballs.start_connection", autospec=True, @@ -1072,6 +1078,7 @@ async def create_connection( async def test_tcp_connector_multiple_hosts_one_timeout( start_connection: mock.Mock, loop: asyncio.AbstractEventLoop, + request_url: str, ) -> None: conn = aiohttp.TCPConnector() @@ -1079,38 +1086,43 @@ async def test_tcp_connector_multiple_hosts_one_timeout( ip2 = "192.168.1.2" ips = [ip1, ip2] ips_tried = [] + ips_success = [] + timeout_error = False + connected = False req = ClientRequest( "GET", - URL("https://mocked.host"), + URL(request_url), loop=loop, ) async def _resolve_host( - host: str, port: int, traces: Optional[Sequence[Trace]] = None + host: str, port: int, traces: object = None ) -> List[ResolveResult]: return [ { "hostname": host, "host": ip, "port": port, - "family": socket.AF_INET, + "family": socket.AF_INET6 if ":" in ip else socket.AF_INET, "proto": 0, "flags": socket.AI_NUMERICHOST, } for ip in ips ] - timeout_error = False - connected = False - - async def create_connection( - *args: object, **kwargs: object - ) -> Tuple[mock.Mock, mock.Mock]: - nonlocal timeout_error, connected + async def start_connection( + addr_infos: Sequence[AddrInfoType], + *, + interleave: Optional[int] = None, + **kwargs: object, + ) -> socket.socket: + nonlocal timeout_error - ip = args[1] + addr_info = addr_infos[0] + addr_info_addr = addr_info[-1] + ip = addr_info_addr[0] ips_tried.append(ip) if ip == ip1: @@ -1118,35 +1130,52 @@ async def create_connection( raise asyncio.TimeoutError if ip == ip2: - connected = True - tr = create_mocked_conn(loop) - pr = create_mocked_conn(loop) + mock_socket = mock.create_autospec( + socket.socket, spec_set=True, instance=True + ) + mock_socket.getpeername.return_value = addr_info_addr + return mock_socket # type: ignore[no-any-return] - def get_extra_info(param: str) -> Union[bool, mock.Mock]: - if param == "sslcontext": - return True + assert False - if param == "ssl_object": - s = create_mocked_conn(loop) - s.getpeercert.return_value = b"foo" - return s + async def create_connection( + *args: object, sock: Optional[socket.socket] = None, **kwargs: object + ) -> Tuple[ResponseHandler, ResponseHandler]: + nonlocal connected - assert False + assert isinstance(sock, socket.socket) + addr_info = sock.getpeername() + ip = addr_info[0] + ips_success.append(ip) + connected = True - tr.get_extra_info = get_extra_info - return tr, pr + # Close the socket since we are not actually connecting + # and we don't want to leak it. + sock.close() + tr = create_mocked_conn(loop) + pr = create_mocked_conn(loop) + return tr, pr - assert False + with mock.patch.object( + conn, "_resolve_host", autospec=True, spec_set=True, side_effect=_resolve_host + ), mock.patch.object( + conn._loop, + "create_connection", + autospec=True, + spec_set=True, + side_effect=create_connection, + ), mock.patch( + "aiohttp.connector.aiohappyeyeballs.start_connection", start_connection + ): + established_connection = await conn.connect(req, [], ClientTimeout()) - with mock.patch.object(conn, "_resolve_host", _resolve_host): - with mock.patch.object(conn._loop, "create_connection", create_connection): - established_connection = await conn.connect(req, [], ClientTimeout()) - assert ips == ips_tried + assert ips_tried == ips + assert ips_success == [ip2] - assert timeout_error - assert connected + assert timeout_error + assert connected - established_connection.close() + established_connection.close() async def test_tcp_connector_resolve_host(loop: asyncio.AbstractEventLoop) -> None: From 001e7cbe405cfa9e9e6fb9493b92016524354109 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 1 Oct 2024 12:08:43 -0500 Subject: [PATCH 08/12] changelog --- CHANGES/7342.breaking.rst | 3 +++ CHANGES/7342.bugfix | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) create mode 100644 CHANGES/7342.breaking.rst delete mode 100644 CHANGES/7342.bugfix diff --git a/CHANGES/7342.breaking.rst b/CHANGES/7342.breaking.rst new file mode 100644 index 00000000000..9997cb9f25e --- /dev/null +++ b/CHANGES/7342.breaking.rst @@ -0,0 +1,3 @@ +Fixed failure to try next host after single-host connection timeout -- by :user:`brettdh`. + +The default ``ClientTimeout`` params has changed to include a ``sock_connect`` timeout of 30 seconds so that this correct behavior happens by default. diff --git a/CHANGES/7342.bugfix b/CHANGES/7342.bugfix deleted file mode 100644 index fa411657e0c..00000000000 --- a/CHANGES/7342.bugfix +++ /dev/null @@ -1,3 +0,0 @@ -Fixed failure to try next host after single-host connection timeout. -Also updated the default ``ClientTimeout`` params to include ``sock_connect`` -so that this correct behavior happens by default. From be1bed5fd9c0d9043d13cb0329015fcb24d81f1f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 1 Oct 2024 13:57:30 -0500 Subject: [PATCH 09/12] fix test --- tests/test_connector.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/test_connector.py b/tests/test_connector.py index 3293c6bfc83..f142105dfb1 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1070,13 +1070,7 @@ async def create_connection( ("https://mocked.host"), ], ) -@mock.patch( - "aiohttp.connector.aiohappyeyeballs.start_connection", - autospec=True, - spec_set=True, -) async def test_tcp_connector_multiple_hosts_one_timeout( - start_connection: mock.Mock, loop: asyncio.AbstractEventLoop, request_url: str, ) -> None: From 6df42a5bf56fc9eb2b7482d7ec3ec991921b2236 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 1 Oct 2024 14:04:06 -0500 Subject: [PATCH 10/12] clienttimeout is documented --- CHANGES/7342.breaking.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/7342.breaking.rst b/CHANGES/7342.breaking.rst index 9997cb9f25e..2a517efcd5b 100644 --- a/CHANGES/7342.breaking.rst +++ b/CHANGES/7342.breaking.rst @@ -1,3 +1,3 @@ Fixed failure to try next host after single-host connection timeout -- by :user:`brettdh`. -The default ``ClientTimeout`` params has changed to include a ``sock_connect`` timeout of 30 seconds so that this correct behavior happens by default. +The default :class:`aiohttp.ClientTimeout` params has changed to include a ``sock_connect`` timeout of 30 seconds so that this correct behavior happens by default. From 50d97cf4e0da8a16079ff6db425d7181300ba683 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 1 Oct 2024 14:06:05 -0500 Subject: [PATCH 11/12] versionchanged --- docs/client_reference.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/client_reference.rst b/docs/client_reference.rst index 9bd0b20784a..347c7d5da60 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -154,6 +154,10 @@ The client session supports the context manager protocol for self closing. .. versionadded:: 3.3 + .. versionchanged:: 3.10.9 + + The default value for the ``sock_connect`` timeout has been changed to 30 seconds. + :param bool auto_decompress: Automatically decompress response body (``True`` by default). .. versionadded:: 2.3 From 6029cd606f24497107efe89c2ddb1bea5e4c76ec Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 1 Oct 2024 14:08:57 -0500 Subject: [PATCH 12/12] Update CHANGES/7342.breaking.rst --- CHANGES/7342.breaking.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES/7342.breaking.rst b/CHANGES/7342.breaking.rst index 2a517efcd5b..1fa511c4c97 100644 --- a/CHANGES/7342.breaking.rst +++ b/CHANGES/7342.breaking.rst @@ -1,3 +1,3 @@ Fixed failure to try next host after single-host connection timeout -- by :user:`brettdh`. -The default :class:`aiohttp.ClientTimeout` params has changed to include a ``sock_connect`` timeout of 30 seconds so that this correct behavior happens by default. +The default client :class:`aiohttp.ClientTimeout` params has changed to include a ``sock_connect`` timeout of 30 seconds so that this correct behavior happens by default.