diff --git a/CHANGES/7342.breaking.rst b/CHANGES/7342.breaking.rst new file mode 100644 index 0000000000..1fa511c4c9 --- /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 client :class:`aiohttp.ClientTimeout` params has changed to include a ``sock_connect`` timeout of 30 seconds so that this correct behavior happens by default. diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 4bc8f5337f..b195486e76 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -60,6 +60,7 @@ Bob Haddleton Boris Feld Boyi Chen Brett Cannon +Brett Higgins Brian Bouterse Brian C. Lane Brian Muller diff --git a/aiohttp/client.py b/aiohttp/client.py index a5c57bb25a..c4e4740102 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -213,7 +213,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) # https://www.rfc-editor.org/rfc/rfc9110#section-9.2.2 IDEMPOTENT_METHODS = frozenset({"GET", "HEAD", "OPTIONS", "TRACE", "PUT", "DELETE"}) diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 31d3c6df08..5947fdc695 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1327,7 +1327,7 @@ async def _create_direct_connection( req=req, client_error=client_error, ) - except ClientConnectorError as exc: + except (ClientConnectorError, asyncio.TimeoutError) as exc: last_exc = exc aiohappyeyeballs.pop_addr_infos_interleave(addr_infos, self._interleave) continue diff --git a/docs/client_quickstart.rst b/docs/client_quickstart.rst index 51b2ca1ec6..f99339cf4a 100644 --- a/docs/client_quickstart.rst +++ b/docs/client_quickstart.rst @@ -417,7 +417,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 05325045ee..8495ecd9d8 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -166,10 +166,14 @@ The client session supports the context manager protocol for self closing. overwrite it on a per-request basis. :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 + .. 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 @@ -898,7 +902,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 f28545b08e..bad4e4a2f6 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -10,7 +10,7 @@ from collections import deque from concurrent import futures from contextlib import closing, suppress -from typing import Any, List, Literal, Optional +from typing import Any, List, Literal, Optional, Sequence, Tuple from unittest import mock import pytest @@ -20,6 +20,7 @@ import aiohttp from aiohttp import client, connector as connector_module, web from aiohttp.client import ClientRequest, ClientTimeout +from aiohttp.client_proto import ResponseHandler from aiohttp.client_reqrep import ConnectionKey from aiohttp.connector import ( _SSL_CONTEXT_UNVERIFIED, @@ -29,6 +30,7 @@ _DNSCacheTable, ) from aiohttp.locks import EventResultOrError +from aiohttp.resolver import ResolveResult from aiohttp.test_utils import make_mocked_coro, unused_port from aiohttp.tracing import Trace @@ -965,7 +967,116 @@ async def create_connection(*args, **kwargs): established_connection.close() -async def test_tcp_connector_resolve_host(loop: Any) -> None: +@pytest.mark.parametrize( + ("request_url"), + [ + ("http://mocked.host"), + ("https://mocked.host"), + ], +) +async def test_tcp_connector_multiple_hosts_one_timeout( + loop: asyncio.AbstractEventLoop, + request_url: str, +) -> None: + conn = aiohttp.TCPConnector() + + ip1 = "192.168.1.1" + ip2 = "192.168.1.2" + ips = [ip1, ip2] + ips_tried = [] + ips_success = [] + timeout_error = False + connected = False + + req = ClientRequest( + "GET", + URL(request_url), + loop=loop, + ) + + async def _resolve_host( + host: str, port: int, traces: object = None + ) -> List[ResolveResult]: + return [ + { + "hostname": host, + "host": ip, + "port": port, + "family": socket.AF_INET6 if ":" in ip else socket.AF_INET, + "proto": 0, + "flags": socket.AI_NUMERICHOST, + } + for ip in ips + ] + + async def start_connection( + addr_infos: Sequence[AddrInfoType], + *, + interleave: Optional[int] = None, + **kwargs: object, + ) -> socket.socket: + nonlocal timeout_error + + addr_info = addr_infos[0] + addr_info_addr = addr_info[-1] + + ip = addr_info_addr[0] + ips_tried.append(ip) + + if ip == ip1: + timeout_error = True + raise asyncio.TimeoutError + + if ip == ip2: + 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] + + assert False + + async def create_connection( + *args: object, sock: Optional[socket.socket] = None, **kwargs: object + ) -> Tuple[ResponseHandler, ResponseHandler]: + nonlocal connected + + assert isinstance(sock, socket.socket) + addr_info = sock.getpeername() + ip = addr_info[0] + ips_success.append(ip) + connected = True + + # 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 + + 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()) + + assert ips_tried == ips + assert ips_success == [ip2] + + assert timeout_error + assert connected + + established_connection.close() + + +async def test_tcp_connector_resolve_host(loop: asyncio.AbstractEventLoop) -> None: conn = aiohttp.TCPConnector(use_dns_cache=True) res = await conn._resolve_host("localhost", 8080)