Fix operator precedence on evicted-session close timeout#3832
Open
genisis0x wants to merge 3 commits intoApeWorX:mainfrom
Open
Fix operator precedence on evicted-session close timeout#3832genisis0x wants to merge 3 commits intoApeWorX:mainfrom
genisis0x wants to merge 3 commits intoApeWorX:mainfrom
Conversation
`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`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong?
HTTPSessionManager.cache_and_return_session(sync) andasync_cache_and_return_session(async) schedule a close-evicted-sessions task to fire after the in-flight request would have timed out, plus a small0.1s slack so any request that started right before the eviction has time to finish before the underlying session is closed.Both call sites used:
+binds tighter thanorin Python, so the expression actually parses asrequest_timeout or (DEFAULT_HTTP_TIMEOUT + 0.1). The intended+ 0.1slack only applied when the caller fell back to the default. Any caller that supplied a non-default timeout (e.g.request_timeout=5) had its close timer fire at exactly5.0s, racing the in-flight request:The comments above each call site already describe the intended behavior (
wait over the default timeout/bit more than the request_timeout).How was it fixed?
Parenthesize the
orso the slack is always applied to the chosen timeout:Same change in both the sync and async paths.
Cute Animal Picture
Type of change
Test plan
Added focused regression tests in
tests/core/utilities/test_http_session_manager.py:test_session_manager_evicted_close_timer_uses_request_timeout_plus_slack— patchesthreading.Timer, fills a 1-entry cache, and asserts the captured interval isrequest_timeout + 0.1.test_session_manager_evicted_close_timer_falls_back_to_default— same path withrequest_timeout=None, asserts the interval isDEFAULT_HTTP_TIMEOUT + 0.1.test_async_evicted_close_task_uses_request_timeout_plus_slack— async equivalent that patches_async_close_evicted_sessionsand asserts it was scheduled withClientTimeout.total + 0.1.The pre-existing
test_async_session_manager_cache_does_not_close_session_before_callalready exercised the async eviction path and continues to pass with the change (it waited for_timeout + 0.1regardless and is not sensitive to the bug).