Skip to content

Commit

Permalink
🐛 fix dangling task waiting for timeout to pass in sync happy eyeballs (
Browse files Browse the repository at this point in the history
  • Loading branch information
Ousret authored Oct 15, 2024
1 parent bd0d634 commit 606670e
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 6 deletions.
5 changes: 5 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -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)
=====================

Expand Down
2 changes: 1 addition & 1 deletion docs/advanced-usage.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down
2 changes: 1 addition & 1 deletion src/urllib3/_version.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# This file is protected via CODEOWNERS
from __future__ import annotations

__version__ = "2.10.904"
__version__ = "2.10.905"
21 changes: 19 additions & 2 deletions src/urllib3/connectionpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Expand Down Expand Up @@ -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 = (
Expand Down
4 changes: 4 additions & 0 deletions src/urllib3/contrib/resolver/in_memory/_dict.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
12 changes: 12 additions & 0 deletions src/urllib3/contrib/resolver/protocols.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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

Expand Down
5 changes: 3 additions & 2 deletions test/with_dummyserver/test_happy_eyeballs.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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(
Expand Down

0 comments on commit 606670e

Please sign in to comment.