diff --git a/CHANGES/7342.breaking.rst b/CHANGES/7342.breaking.rst new file mode 100644 index 00000000000..1fa511c4c97 --- /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 fbfef32f398..e18aad14f64 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -63,6 +63,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 1cdb046c460..f31ad6bad25 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -214,7 +214,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 1237f3c74d4..e54ac015e19 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -1243,7 +1243,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 eea1bba9a54..b731a80ab9d 100644 --- a/docs/client_quickstart.rst +++ b/docs/client_quickstart.rst @@ -408,7 +408,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 4e6224c78bd..347c7d5da60 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -150,10 +150,14 @@ 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 + .. 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 @@ -882,7 +886,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 f8d58e82666..f142105dfb1 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -1063,6 +1063,115 @@ async def create_connection( established_connection.close() +@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)