Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 091781519c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return conn.read_response( | ||
| disconnect_on_error=False, push_request=True, timeout=read_timeout | ||
| ) | ||
| except TimeoutError: |
There was a problem hiding this comment.
When RESP3 is used and push_handler_func raises redis.exceptions.TimeoutError (for example, its own Redis I/O times out), Connection.read_response() routes that callback exception through this catch too, so nonblocking get_message() returns None rather than surfacing the handler failure. The Python RESP3 parser rewinds the frame on that exception, causing each poll to invoke the same failing handler again, while hiredis has already consumed the frame and silently drops the failure/message; the async PubSub path still propagates Redis TimeoutError. Narrow the suppression to a timeout produced by the bounded socket read.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. A TimeoutError from a push_handler_func reaches try_read() the same way as a read timeout, so the two cannot be told apart there without relying on the implicit exception chain. With a push handler installed, the timeout now goes to the retry layer as on master; only reads without one return None. test_get_message_timeout_with_push_handler_reaches_retry_layer covers this.
One detail: on master, with the default retry, the handler failure is not surfaced either. The retry layer reconnects and the next read returns the resubscribe reply, so the failing message and the one after it are lost without an error. The regression was elsewhere: the pure-Python parser ran a handler that keeps failing again on every poll, so the subscriber spun without delivering anything. With this change the push handler case behaves as on master.
| returned by a later call, not dropped by a reconnect. | ||
| """ | ||
| p = r.pubsub() | ||
| p.subscribe("foo") |
There was a problem hiding this comment.
In the cluster test profile, this unmarked test shares foo with many other cases. The existing wait_for_handled_message helper (lines 72-75) and adjacent test_timeout_with_pattern_subscribe explicitly account for an earlier PUBLISH foo arriving late over the cluster bus; if that happens after this subscription, can_read() observes the stale frame and the final b"hello" assertion fails even though resume behavior is correct. Use a channel unique to this test for both calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the test now uses its own channel, partly-received.
Description of change
Since #3982, the sync
PubSub.get_message()passes its timeout toread_response(). If only part of a message has arrived, the read times out withTimeoutError._execute()hands that to the retry layer, which calls_reconnect(): the connection is closed and resubscribed, and the partly read message (plus anything published during the reconnect) is lost. The caller just getsNone.Impact
get_message()with the defaulttimeout=0,run_in_thread()with the defaultsleep_time=0.0, and keyspace notifications built on them.timeout=0andtimeout=0.01.run_in_thread()) narrows the window but does not close it.Fix
For non-blocking reads,
try_read()now returnsNoneonTimeoutErrorinstead of letting it reach the retry layer. The parser keeps what it has read, so the next call returns the whole message. With apush_handler_func, theTimeoutErrormay come from the handler instead, so that case still goes to the retry layer as on master.get_message()still never hangs, which was the goal of #3982, and blocking reads still raise. The asyncio client already works this way.Tests
test_get_message_resumes_a_partly_received_messageinTestPubSubTimeoutPropagation. It makes one read raiseTimeoutError, as the parser does for an incomplete message, then checks that there is no reconnect and that the next call returns the message. It fails on master and passes with the fix. Resuming a partly read reply is already covered for each parser bytest_connection_parse_response_resume, so the new test mocksread_response()and checks only the PubSub side; a version with a real 1 MB message failed on master only about half the time on Linux.test_get_message_timeout_with_push_handler_reaches_retry_layerchecks that with a push handler the timeout still reaches the retry layer. It fails if that case is suppressed too.redislabs/client-libs-test:8.10.3(standalone and a 6-node cluster; no TLS, sentinel or replica), with and without hiredis, on RESP2 and RESP3. In all 8 runs the failing tests match master, apart from the new test, which passes. After the push handler change I rerantests/test_pubsub.pyandtests/test_keyspace_notifications.pyin the same 8 configurations and thefixed_clientsubset, with the same result.invoke linterspasses.Pull Request check-list