From 606670edb9c72bbc46d1e243339860c03024971a Mon Sep 17 00:00:00 2001 From: "TAHRI Ahmed R." Date: Tue, 15 Oct 2024 08:58:18 +0200 Subject: [PATCH] :bug: fix dangling task waiting for timeout to pass in sync happy eyeballs (#160) --- CHANGES.rst | 5 +++++ docs/advanced-usage.rst | 2 +- src/urllib3/_version.py | 2 +- src/urllib3/connectionpool.py | 21 +++++++++++++++++-- .../contrib/resolver/in_memory/_dict.py | 4 ++++ src/urllib3/contrib/resolver/protocols.py | 12 +++++++++++ test/with_dummyserver/test_happy_eyeballs.py | 5 +++-- 7 files changed, 45 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index e66abd0db5..b58a7706c6 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,3 +1,8 @@ +2.10.905 (2024-10-15) +===================== + +- Fixed dangling task waiting for timeout when using Happy Eyeballs in a synchronous context. + 2.10.904 (2024-10-13) ===================== diff --git a/docs/advanced-usage.rst b/docs/advanced-usage.rst index 293bb82a4a..edc125c9f8 100644 --- a/docs/advanced-usage.rst +++ b/docs/advanced-usage.rst @@ -1442,7 +1442,7 @@ Happy Eyeballs requires to do concurrent tasks. .. note:: The asynchronous algorithm is more efficient than its synchronous counterpart. -.. warning:: A particularity exist in the synchronous context, a timeout is mandatory due its nature (threads: cannot kill them if pending I/O). By default we expect a max delay of 400ms at most. +.. warning:: A particularity exist in the synchronous context, a timeout is mandatory due its nature (threads: cannot kill them if pending I/O). By default we expect a max delay of 5000ms at most if no connect timeout is specified. In what scenario do I gain using this? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/urllib3/_version.py b/src/urllib3/_version.py index 637543e451..9d2b08ebb4 100644 --- a/src/urllib3/_version.py +++ b/src/urllib3/_version.py @@ -1,4 +1,4 @@ # This file is protected via CODEOWNERS from __future__ import annotations -__version__ = "2.10.904" +__version__ = "2.10.905" diff --git a/src/urllib3/connectionpool.py b/src/urllib3/connectionpool.py index c8bda0571a..2136976abb 100644 --- a/src/urllib3/connectionpool.py +++ b/src/urllib3/connectionpool.py @@ -1894,7 +1894,7 @@ def _new_conn(self, *, heb_timeout: Timeout | None = None) -> HTTPSConnection: heb_timeout.connect_timeout if heb_timeout.connect_timeout is not None and isinstance(heb_timeout.connect_timeout, (float, int)) - else 0.4 + else 5.0 ) for ip_address in ip_addresses[:max_task]: @@ -1966,10 +1966,27 @@ def _happy_eyeballs_completed(t: Future[None]) -> None: if task == winning_task: continue + associated_conn = challengers[tasks.index(task)] + if task.running(): + # dangling TCP conn + try: + if associated_conn._resolver._sock_cursor: + associated_conn._resolver._sock_cursor.shutdown(0) + associated_conn._resolver._sock_cursor.close() + # dangling UDP conn (they usually stuck later in the process) + elif associated_conn.sock: + associated_conn.sock.shutdown(0) + associated_conn.sock.close() + associated_conn.sock = ( + None # ensure it's not used to send close frames + ) + except OSError: + pass # ignore any error + associated_conn.close() task.cancel() else: - challengers[tasks.index(task)].close() + associated_conn.close() if winning_task is None: within_delay_msg: str = ( diff --git a/src/urllib3/contrib/resolver/in_memory/_dict.py b/src/urllib3/contrib/resolver/in_memory/_dict.py index 103d5071b3..d46a46ee19 100644 --- a/src/urllib3/contrib/resolver/in_memory/_dict.py +++ b/src/urllib3/contrib/resolver/in_memory/_dict.py @@ -30,6 +30,10 @@ def __init__(self, *patterns: str, **kwargs: typing.Any): self.register(hostname, addr) self._host_patterns = tuple([]) + # probably about our happy eyeballs impl (sync only) + if len(self._hosts) == 1 and len(self._hosts[list(self._hosts.keys())[0]]) == 1: + self._unsafe_expose = True + def recycle(self) -> BaseResolver: return self diff --git a/src/urllib3/contrib/resolver/protocols.py b/src/urllib3/contrib/resolver/protocols.py index 8dad0305c7..bebff29486 100644 --- a/src/urllib3/contrib/resolver/protocols.py +++ b/src/urllib3/contrib/resolver/protocols.py @@ -64,6 +64,11 @@ def __init__( if not self._host_patterns and "patterns" in kwargs: self._host_patterns = kwargs["patterns"] + # allow to temporarily expose a sock that is "being" created + # this helps with our Happy Eyeballs implementation in sync. + self._unsafe_expose: bool = False + self._sock_cursor: socket.socket | None = None + def recycle(self) -> BaseResolver: if self.is_available(): raise RuntimeError("Attempting to recycle a Resolver that was not closed") @@ -221,7 +226,14 @@ def create_connection( sock.settimeout(timeout) if source_address: sock.bind(source_address) + + if self._unsafe_expose: + self._sock_cursor = sock + sock.connect(sa) + + if self._unsafe_expose: + self._sock_cursor = None # Break explicitly a reference cycle err = None diff --git a/test/with_dummyserver/test_happy_eyeballs.py b/test/with_dummyserver/test_happy_eyeballs.py index 1cfebc1ffd..9d4b6062e1 100644 --- a/test/with_dummyserver/test_happy_eyeballs.py +++ b/test/with_dummyserver/test_happy_eyeballs.py @@ -111,7 +111,8 @@ def test_with_one_tarpit(self, ipv6_san_server: ServerConfig) -> None: def test_with_all_tarpit_implicit_timeout(self) -> None: """In the synchronous context, using HEB algorithm, we cannot set the infinite wait. - We have to make sure it fails after a short period of time (by default 400ms)""" + We have to make sure it fails after a short period of time (by default 5000ms) + """ with PoolManager( happy_eyeballs=True, resolver="in-memory://default?hosts=dummy.io:240.0.0.0,dummy.io:240.0.0.1", @@ -125,7 +126,7 @@ def test_with_all_tarpit_implicit_timeout(self) -> None: "No suitable address to connect to using Happy Eyeballs" in exc.value.args[0] ) - assert "within 0.4s" in exc.value.args[0] + assert "within 5.0s" in exc.value.args[0] def test_with_all_tarpit_explicit_timeout_global(self) -> None: with PoolManager(