(fix): Return a response that arrived instead of reporting a timeout - #330
Open
richardssam wants to merge 1 commit into
Open
Conversation
Collaborator
|
Hi @richardssam - thanks for this. I can't merge because the commits haven't been 'signed off', though. Please could you have a look. The '...' button on the far right side of the DCO check immediately above this comment tells you how to resolve it. |
_dequeue_messages files every response it dequeues into self.responses, whatever request it was watching for - only its break is specific to watch_for. response() then let TimeoutError propagate without consulting self.responses, so when any other consumer of the connection's queue took our response first, the answer was stored correctly and the caller was told the request timed out. This is not a rare race. Connection(background_processing=True) starts exactly such a consumer via process_events_forever, and with one running, 20 of 20 bounded reads raised TimeoutError with the answer already recorded. It also fires with no threads at all: a request issued from inside a broadcast callback re-enters the same pump on the calling thread, so an outer frame can file the answer while the inner call concludes it timed out. It matters beyond a spurious exception, because callers treat TimeoutError as evidence that an actor is unresponsive and act on it - dropping cached handles, re-acquiring, marking a peer unhealthy. Re-check self.responses before propagating. Kept at the response() level rather than in _dequeue_messages, whose break condition is correct: the loop's job is to pump until it sees what it was told to watch for. What was wrong is concluding "no answer" from "I did not dequeue it myself". A genuine timeout, where no response was ever recorded, still raises. Signed-off-by: Sam.Richards@taurich.org <Sam.Richards@taurich.org>
richardssam
force-pushed
the
pr/python-connection-response-timeout
branch
from
September 5, 2026 09:08
b9ac122 to
994fe36
Compare
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.
Linked issues
Fixes #331
Summarize your change.
Connection.response()now re-checksself.responsesbefore lettingTimeoutErrorpropagate, and returns the recorded answer if one is there.Two lines of behaviour change, plus a regression test in
python/test/test_connection.py.Describe the reason for the change.
_dequeue_messagesfiles every response it dequeues intoself.responses,whatever request it was watching for — only its
breakis specific towatch_for.response()letTimeoutErrorpropagate without consultingself.responses, so when any other consumer of the connection's queue took ourresponse first, the answer was stored correctly and the caller was told the
request timed out.
This is not a rare race:
Connection(background_processing=True)starts exactly such a consumer viaprocess_events_forever. With one running, 20 of 20 bounded reads raisedTimeoutErrorwith the answer already recorded.callback re-enters the same pump on the calling thread, so an outer frame can
file the answer while the inner call concludes it timed out (measured 1 in 20).
It matters beyond a spurious exception, because callers treat
TimeoutErrorasevidence that an actor is unresponsive and act on it — dropping cached handles,
re-acquiring, marking a peer unhealthy.
The change is kept at the
response()level rather than in_dequeue_messages,whose break condition is correct: the loop's job is to pump until it sees what it
was told to watch for. What was wrong is concluding "no answer" from "I did not
dequeue it myself".
Describe what you have tested and on which operating system.
macOS 15 (arm64),
develop@cb0e9b6bbuilt from source, Python 3.11.python/test/test_connection.pyadds two tests:test_response_already_received_is_not_a_timeout— fails without this change,passes with it
test_genuine_timeout_still_raises— passes both before and after, guardingagainst "fixing" the first by swallowing real timeouts
Full
python/testsuite, same command and environment, bundle swapped betweenstock
developand this change:test_response_already_received_is_not_a_timeoutThe delta is exactly the one test. The 11 other failures are pre-existing and
environmental in a manual run — 6 are
KeyError: 'TEST_RESOURCE'(a variable theCTest wrapper sets), and 4
test_sessioncount assertions cascade from those.Also exercised against a live session with a standalone repro covering the
threaded case, the callback-reentrancy case, and event delivery under load.
Add a list of changes, and note any that might need special attention during the review.
python/src/xstudio/connection/__init__.py—response()catchesTimeoutErrorand re-raises only whenself.responses.get(req_id) is Nonepython/test/test_connection.py— new file, two testsWorth attention: this is a behaviour change on an error path, which is where
assumptions hide. A caller that previously saw
TimeoutErrormay now get aresponse. That is the intent — no caller can be relying on losing an answer it
did in fact receive — but it is the thing to sanity-check.
_dequeue_messagesis deliberately untouched.