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. diff --git a/tests/core/utilities/test_http_session_manager.py b/tests/core/utilities/test_http_session_manager.py index 1621c5f5f1..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, ) @@ -238,6 +241,48 @@ 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. + 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 +376,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, ) )