fix(pubsub): add timeout parameter to listen() to prevent Redis 8.0 timeout errors#4101
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 95c21e6. Configure here.
|
@cursor[bot] Thanks for the review! I've pushed a fix that addresses all three issues:
Please take another look when you have a chance. |
|
Hi @goingforstudying-ctrl, thank you for your contribution! I'll take a look at it shortly. |
|
Pushed a follow-up fix for the cursor bot review comments on #4101:
Let me know if anything else is needed. |
|
Pushed a fix that tracks remaining timeout across loop iterations and passes it to parse_response instead of the full timeout each time. This should address the cumulative timeout drift issue. Let me know if anything else is needed. |
…imeout errors - Sync PubSub.listen() now accepts an optional timeout parameter - Async PubSub.listen() now accepts an optional timeout parameter - Both delegate to parse_response(block=(timeout is None), timeout=timeout) - Added 3 tests covering timeout=0.1, timeout=1.0 with message, timeout=None Fixes redis#4098
- Change default timeout from 0.0 to None to preserve backward-compatible blocking behavior and prevent busy-looping when listen() is called without arguments. - Add elapsed-time tracking so that listen(timeout=X) breaks out of the loop after the timeout expires, preventing infinite iteration when no message arrives. Fixes cursor[bot] review comments: 1. 'Listen default timeout busy loops' — defaulting to 0.0 caused parse_response(block=False) to return None immediately, spinning the CPU. 2. 'Finite listen timeout never ends' — without elapsed-time tracking, the while self.subscribed loop would keep calling parse_response indefinitely even after the timeout period.
…timeout drift - Track remaining timeout across loop iterations instead of passing the full timeout each time. This ensures listen(timeout=N) returns within approximately N seconds total, not N seconds per iteration. - Addresses cursor[bot] review comments about full timeout being passed each loop iteration.
goingforstudying-ctrl
force-pushed
the
fix/pubsub-listen-timeout
branch
from
5586cea to
e9cb71b
Compare
|
Closing this PR as all the changes are added in #4103 |
2 participants
Fixes #4098
Adds an optional parameter to (sync and async) that is forwarded to , matching the existing API. This prevents on Redis 8.0.0+ when no messages arrive within the connection's (default 5s).
Note
Low Risk
Backward-compatible API default; behavior change is limited to pubsub listen loops and read timeout handling, with dedicated tests.
Overview
Adds an optional
timeoutargument toPubSub.listen()in both sync and async clients (fixes #4098). Previouslylisten()always blocked indefinitely viaparse_response(block=True), which could surface connection read timeouts on Redis 8.0+ when idle. With a timeout, it now mirrorsget_message(): non-blocking reads with a shrinkingremainingbudget usingtime.monotonic(), then exits the generator when the budget is exhausted;timeout=Nonekeeps indefinite blocking.New tests cover empty
listen(timeout=…), delivering a message within the window, andtimeout=Noneblocking until publish.Reviewed by Cursor Bugbot for commit e9cb71b. Bugbot is set up for automated code reviews on this repo. Configure here.