From 1ac680d2086df816dc3fc0621827fd9a2edeabf7 Mon Sep 17 00:00:00 2001 From: genisis0x Date: Fri, 1 May 2026 16:47:24 +0530 Subject: [PATCH 1/3] Fix operator precedence on evicted-session close timeout `HTTPSessionManager.cache_and_return_session` schedules `_close_evicted_sessions` (sync) and `_async_close_evicted_sessions` (async) to fire after the in-flight request would time out, plus a small `0.1`s slack so any request that started right before the eviction has time to finish. Both call sites used: request_timeout or DEFAULT_HTTP_TIMEOUT + 0.1 Python parses `+` tighter than `or`, so this is `request_timeout or (DEFAULT_HTTP_TIMEOUT + 0.1)`. When a caller supplied a non-default timeout (e.g. 5s), the close timer fired at exactly the request timeout instead of `request_timeout + 0.1`, and the evicted session could be closed out from under an in-flight request. The intended slack only kicked in when the caller relied on the default. Parenthesize the expression so the `+ 0.1` slack is always applied: (request_timeout or DEFAULT_HTTP_TIMEOUT) + 0.1 Adds focused regression tests for both sync and async paths that patch the timer / scheduling primitive and assert the captured interval is `request_timeout + 0.1`. --- .../utilities/test_http_session_manager.py | 87 +++++++++++++++++++ web3/_utils/http_session_manager.py | 11 ++- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/tests/core/utilities/test_http_session_manager.py b/tests/core/utilities/test_http_session_manager.py index 1621c5f5f1..e6eead4995 100644 --- a/tests/core/utilities/test_http_session_manager.py +++ b/tests/core/utilities/test_http_session_manager.py @@ -238,6 +238,54 @@ def test_session_manager_precached_session(mocker, http_session_manager): assert adapter._pool_maxsize == 100 +def test_session_manager_evicted_close_timer_uses_request_timeout_plus_slack( + mocker, http_session_manager +): + # The evicted-session close timer should fire at ``request_timeout + 0.1`` + # so any in-flight request has slack to finish before its session is closed. + # An earlier version dropped the slack for non-default timeouts because of + # operator precedence (``x or DEFAULT + 0.1`` parses as ``x or (DEFAULT + 0.1)``). + timer_mock = mocker.patch( + "web3._utils.http_session_manager.threading.Timer" + ) + + http_session_manager.session_cache = SimpleCache(1) + http_session_manager.cache_and_return_session( + URI("https://www.test1.com"), request_timeout=5.0 + ) + http_session_manager.cache_and_return_session( + URI("https://www.test2.com"), request_timeout=5.0 + ) + + # eviction triggered the timer with the right interval + assert timer_mock.called + interval = timer_mock.call_args.args[0] + assert interval == pytest.approx(5.1) + + +def test_session_manager_evicted_close_timer_falls_back_to_default( + mocker, http_session_manager +): + timer_mock = mocker.patch( + "web3._utils.http_session_manager.threading.Timer" + ) + + http_session_manager.session_cache = SimpleCache(1) + http_session_manager.cache_and_return_session( + URI("https://www.test1.com"), request_timeout=None + ) + http_session_manager.cache_and_return_session( + URI("https://www.test2.com"), request_timeout=None + ) + + assert timer_mock.called + interval = timer_mock.call_args.args[0] + # ``DEFAULT_HTTP_TIMEOUT`` is 30 in this codebase; verify we still add the slack. + from web3._utils.http import DEFAULT_HTTP_TIMEOUT + + assert interval == pytest.approx(DEFAULT_HTTP_TIMEOUT + 0.1) + + def test_simple_cache_cache_session(): cache = SimpleCache(2) _, evicted_items = cache.cache("1", "Hello1") @@ -331,6 +379,45 @@ def raise_for_status() -> None: pass +@pytest.mark.asyncio +async def test_async_evicted_close_task_uses_request_timeout_plus_slack( + mocker, http_session_manager +): + # Async equivalent of the sync precedence-bug regression test: the + # evicted-session close coroutine should be scheduled with + # ``request_timeout.total + 0.1``. + captured: dict[str, float] = {} + + async def _capture(timeout, evicted_sessions): + captured["timeout"] = timeout + + mocker.patch.object( + http_session_manager, + "_async_close_evicted_sessions", + side_effect=_capture, + ) + + http_session_manager.session_cache = SimpleCache(1) + timeout = ClientTimeout(total=5.0) + + s1 = ClientSession() + s2 = ClientSession() + try: + await http_session_manager.async_cache_and_return_session( + URI("https://www.test1.com"), session=s1, request_timeout=timeout + ) + await http_session_manager.async_cache_and_return_session( + URI("https://www.test2.com"), session=s2, request_timeout=timeout + ) + # let the scheduled close-task run + await asyncio.sleep(0) + finally: + await s1.close() + await s2.close() + + assert captured.get("timeout") == pytest.approx(5.1) + + @pytest.mark.asyncio async def test_session_manager_async_json_make_get_request( mocker, http_session_manager diff --git a/web3/_utils/http_session_manager.py b/web3/_utils/http_session_manager.py index 913ea550f1..f99d164292 100644 --- a/web3/_utils/http_session_manager.py +++ b/web3/_utils/http_session_manager.py @@ -95,8 +95,10 @@ def cache_and_return_session( ) threading.Timer( # If `request_timeout` is `None`, don't wait forever for the closing - # session to finish the request. Instead, wait over the default timeout. - request_timeout or DEFAULT_HTTP_TIMEOUT + 0.1, + # session to finish the request. Instead, wait over the default + # timeout. Parenthesize the `or` so the `+ 0.1` slack is applied to + # the chosen timeout rather than only to the fallback. + (request_timeout or DEFAULT_HTTP_TIMEOUT) + 0.1, self._close_evicted_sessions, args=[evicted_sessions], ).start() @@ -267,8 +269,9 @@ async def async_cache_and_return_session( self._async_close_evicted_sessions( # if `ClientTimeout.total` is `None`, don't wait forever for the # closing session to finish the request. Instead, use the default - # timeout. - request_timeout.total or DEFAULT_HTTP_TIMEOUT + 0.1, + # timeout. Parenthesize the `or` so the `+ 0.1` slack is applied + # to the chosen timeout rather than only to the fallback. + (request_timeout.total or DEFAULT_HTTP_TIMEOUT) + 0.1, evicted_sessions, ) ) From 37f1bfb5cdfe20d314f793ffb9e839b880d7d4b4 Mon Sep 17 00:00:00 2001 From: genisis0x Date: Fri, 1 May 2026 16:48:05 +0530 Subject: [PATCH 2/3] Add newsfragment for #3832 --- newsfragments/3832.bugfix.rst | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 newsfragments/3832.bugfix.rst diff --git a/newsfragments/3832.bugfix.rst b/newsfragments/3832.bugfix.rst new file mode 100644 index 0000000000..0cafddc5d4 --- /dev/null +++ b/newsfragments/3832.bugfix.rst @@ -0,0 +1,6 @@ +Fix an operator-precedence bug in ``HTTPSessionManager`` so the +``+ 0.1`` slack on the evicted-session close timer is applied to the +caller-supplied ``request_timeout`` rather than only to the +``DEFAULT_HTTP_TIMEOUT`` fallback. Previously a non-default timeout +caused the close timer to fire at exactly the request timeout, racing +the in-flight request. From a801af8691ee3d24303d055692bdcb74271577e1 Mon Sep 17 00:00:00 2001 From: genisis0x Date: Fri, 1 May 2026 16:53:10 +0530 Subject: [PATCH 3/3] Apply pre-commit (black, isort) on test additions --- tests/core/utilities/test_http_session_manager.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/core/utilities/test_http_session_manager.py b/tests/core/utilities/test_http_session_manager.py index e6eead4995..0ab815e745 100644 --- a/tests/core/utilities/test_http_session_manager.py +++ b/tests/core/utilities/test_http_session_manager.py @@ -26,6 +26,9 @@ from web3._utils.caching import ( generate_cache_key, ) +from web3._utils.http import ( + DEFAULT_HTTP_TIMEOUT, +) from web3._utils.http_session_manager import ( HTTPSessionManager, ) @@ -245,9 +248,7 @@ def test_session_manager_evicted_close_timer_uses_request_timeout_plus_slack( # so any in-flight request has slack to finish before its session is closed. # An earlier version dropped the slack for non-default timeouts because of # operator precedence (``x or DEFAULT + 0.1`` parses as ``x or (DEFAULT + 0.1)``). - timer_mock = mocker.patch( - "web3._utils.http_session_manager.threading.Timer" - ) + timer_mock = mocker.patch("web3._utils.http_session_manager.threading.Timer") http_session_manager.session_cache = SimpleCache(1) http_session_manager.cache_and_return_session( @@ -266,9 +267,7 @@ def test_session_manager_evicted_close_timer_uses_request_timeout_plus_slack( def test_session_manager_evicted_close_timer_falls_back_to_default( mocker, http_session_manager ): - timer_mock = mocker.patch( - "web3._utils.http_session_manager.threading.Timer" - ) + timer_mock = mocker.patch("web3._utils.http_session_manager.threading.Timer") http_session_manager.session_cache = SimpleCache(1) http_session_manager.cache_and_return_session( @@ -281,8 +280,6 @@ def test_session_manager_evicted_close_timer_falls_back_to_default( assert timer_mock.called interval = timer_mock.call_args.args[0] # ``DEFAULT_HTTP_TIMEOUT`` is 30 in this codebase; verify we still add the slack. - from web3._utils.http import DEFAULT_HTTP_TIMEOUT - assert interval == pytest.approx(DEFAULT_HTTP_TIMEOUT + 0.1)