Fix PubSub timeout propagation to prevent indefinite hangs on socket read operations#3982
Conversation
…opagated down to the socket read. This improves pubsub behaviour - now the timeout is applied when waiting for messages.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in Redis PubSub where get_message(timeout=X) could hang indefinitely. The issue was that while can_read() respected the timeout, the subsequent read_response() call did not receive it, causing indefinite blocking on socket reads when partial data or connection issues occurred.
Changes:
- Added timeout parameter propagation through the entire sync read chain:
get_message()→parse_response()→read_response()→ parser methods → socket buffer operations - Moved SENTINEL constant from
redis/connection.pytoredis/_parsers/socket.pyfor broader accessibility - Added comprehensive test coverage (887 lines) validating timeout behavior across all PubSub variants and edge cases
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| redis/_parsers/socket.py | Added timeout parameter to read() and readline() methods; made SENTINEL constant available for import |
| redis/_parsers/resp2.py | Added timeout parameter to sync parser's read_response() and _read_response() methods |
| redis/_parsers/resp3.py | Added timeout parameter to sync parser's read_response() and _read_response() methods |
| redis/_parsers/hiredis.py | Added timeout parameter to read_response() method and propagated to read_from_socket() |
| redis/connection.py | Added timeout parameter to abstract and concrete read_response() methods; moved SENTINEL to _parsers.socket |
| redis/sentinel.py | Added timeout parameter to SentinelManagedConnection.read_response() |
| redis/client.py | Updated parse_response() to properly propagate timeout to read_response(); added comprehensive docstring |
| redis/asyncio/client.py | Added comprehensive docstring to parse_response() (async already had timeout support) |
| redis/cluster.py | Updated _sharded_message_generator() to accept and propagate timeout parameter |
| tests/test_sentinel_managed_connection.py | Added 107 lines of tests for timeout parameter propagation in SentinelManagedConnection |
| tests/test_pubsub.py | Added 280 lines of sync PubSub timeout tests covering standard and cluster scenarios |
| tests/test_connection.py | Added 115 lines of tests for CacheProxyConnection timeout parameter propagation |
| tests/test_asyncio/test_pubsub.py | Added 192 lines of async PubSub timeout tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
🛡️ Jit Security Scan Results✅ No security findings were detected in this PR
Security scan by Jit
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
🛠 Fix: Prevent Indefinite Hang in
PubSub.get_message()with Timeouts🚨 Problem
This PR fixes a critical hang issue in
PubSub.get_message()when using timeouts.Previously:
can_read()correctly respected the providedtimeout.read_response()call did not receive the timeout.❗ Consequence
This created a scenario where:
can_read()reported that data was available.read_response()attempted to read from the socket.This resulted in unexpected and indefinite hangs.
✅ Solution
This PR propagates an optional
timeoutparameter through the entire call chain:get_message()
→ read_response()
→ parser layer
→ connection layer
→ socket-level recv()
Coverage Includes
All parser implementations
All connection types
Both execution models
The timeout is now consistently respected down to the lowest-level socket
recv()call.🧪 Test Coverage
This PR adds comprehensive test coverage (887 lines) validating timeout behavior across:
timeout=0timeout=NoneAll relevant sync and async paths are covered.
🎯 Result
PubSub.get_message()now behaves correctly and predictably when timeouts are used, eliminating the risk of indefinite blocking under partial-read or connection edge cases.Note
Medium Risk
Touches core response parsing and socket read paths (RESP2/RESP3/Hiredis and
Connection.read_response), so mistakes could cause new timeouts or regressions in message parsing under load. Added/updated tests reduce risk, but behavior changes affect all PubSub and push-response reads.Overview
Fixes Pub/Sub timeouts so
get_message(timeout=...)can’t hang indefinitely by threading an explicittimeoutvalue throughPubSub.parse_response()→Connection.read_response()→ parser implementations →SocketBuffersocket reads.Adds a
timeoutparameter (defaulting toSENTINELto preserve existing socket timeout behavior) across RESP2/RESP3/Hiredis parsers,SocketBuffer.read/readline,SentinelManagedConnection, and cache proxy connections, and propagates the timeout in cluster sharded Pub/Sub message polling.Expands test coverage with new sync/async PubSub and cluster sharded PubSub timeout-propagation tests, plus unit tests verifying timeout passthrough for proxy and sentinel connections.
Written by Cursor Bugbot for commit 32c19a0. This will update automatically on new commits. Configure here.