Fix blocking commands timeout when blocking_timeout exceeds socket_timeout

[C1-BA-B1-F3]

Problem

Blocking commands like BRPOP, BLPOP, BRPOPLPUSH, BLMOVE, and BLMPOP fail with TimeoutError when the command's blocking timeout exceeds the client's socket_timeout.

Issue: #2807

Root Cause

Blocking commands send the timeout to the Redis server but do not pass it to the connection's read_response() method. The parser already supports custom timeouts via the timeout parameter, but it wasn't being used for blocking commands.

For example, with socket_timeout=4 and brpop('key', timeout=8):

1. Client sends BRPOP key 8 to server
2. Client waits for response with 4-second socket timeout
3. Socket times out after 4 seconds, before the server can respond (after 8 seconds)

Fix

This fix:

• Adds _blocking_timeout option to all blocking list commands
• Modifies parse_response() in sync, async, and cluster clients to extract _blocking_timeout and pass it to connection.read_response()
• The socket buffer temporarily uses the blocking timeout for the read operation, preventing premature socket timeouts

Changes

• redis/commands/core.py: Added _blocking_timeout parameter to blpop, brpop, brpoplpush, blmove, blmpop
• redis/client.py: Modified parse_response() to extract and pass _blocking_timeout
• redis/asyncio/client.py: Same change for async client
• redis/asyncio/cluster.py: Same change for async cluster client

Testing

Verified with unit tests that:

1. Blocking commands pass _blocking_timeout correctly
2. parse_response() correctly extracts and passes timeout to connection.read_response()
3. Non-blocking commands use default SENTINEL timeout (no behavior change)

---

Note

Medium Risk
Touches core command execution and socket read timeouts across sync/async/cluster clients; behavior change is targeted to blocking commands but incorrect timeout mapping could affect long-blocking or indefinite waits.

Overview
Fixes premature TimeoutError on blocking list commands when the Redis wait time is longer than the client socket_timeout.

Blocking commands (BLPOP, BRPOP, BRPOPLPUSH, BLMOVE, BLMPOP) now pass an internal _blocking_timeout through execute_command, and sync, async, and async cluster parse_response() forward it to connection.read_response(timeout=…) so the read waits at least as long as the server block. Redis timeout=0 (block forever) is mapped to None on sync reads and math.inf on async reads to match each stack’s timeout semantics.

Adds offline async unit tests for parse_response timeout forwarding and the zero-timeout mapping.

^{Reviewed by Cursor Bugbot for commit 8f30074. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit efce672. Configure here.}

[petyaslavova]

Please clean up the branch from unrelated changes.

[C1-BA-B1-F3]

Branch cleaned up @petyaslavova! Rebased onto upstream master, only the blocking timeout commits remain. Unrelated hiredis/FT.INFO changes removed.

[simonhammes]

Is xreadgroup() also affected by this when using the block keyword argument?

I'm seeing regular timeout errors after upgrading to v8.0.1. I did not change anything else.

EDIT: FTR, I used block=5000. Setting socket_timeout to 10 fixed this issue for me.

[C1-BA-B1-F3]

@simonhammes Good catch — you're right that xreadgroup is affected. This PR currently scopes _blocking_timeout only to the list blocking commands (BLPOP, BRPOP, BRPOPLPUSH, BLMOVE, BLMPOP). XREAD/XREADGROUP take a block argument but they call execute_command("XREAD"/"XREADGROUP", ...) without forwarding _blocking_timeout, so they still hit the same premature socket_timeout behavior when block > socket_timeout.

So yes: with block=5000 and a low socket_timeout, your xreadgroup reads can time out early — setting socket_timeout=10 (as you found) is the current workaround.

Do you want me to extend this fix to XREAD/XREADGROUP in this PR, or track it as a follow-up? I'm happy to add it here since the mechanism (parse_response reading _blocking_timeout) would be identical.

[eeshsaxena]

The overall approach (thread the blocking command's own timeout through to the socket read so socket_timeout doesn't abort it) is right, but I think the timeout == 0 -> None mapping is incorrect on the async path, and it's exactly the "block indefinitely" case.

redis/asyncio/connection.py::read_response documents its timeout semantics as:

• None (default): fall back to self.socket_timeout.
• math.inf: block indefinitely with no timeout.
• float: apply that timeout for this single read.

So on the async side, mapping the server-side "block forever" value (BLPOP key 0) to None:

blocking_timeout = options.pop("_blocking_timeout", None)
if blocking_timeout == 0:
    blocking_timeout = None
...
response = await connection.read_response(timeout=blocking_timeout)

makes the read fall back to self.socket_timeout rather than blocking indefinitely. With a socket_timeout configured, BLPOP key 0 (or BLPOP with timeout 0) would abort after socket_timeout instead of waiting forever - which is the very bug this PR is trying to fix, just for the timeout=0 case. Per that docstring, the async block-forever value should be math.inf, not None.

The sync path happens to be fine, but for a different reason: sync read_response defaults to SENTINEL (= use configured socket_timeout) and timeout=None there flows down to settimeout(None) = blocking mode, so 0 -> None does block forever. That asymmetry (async None = "use socket_timeout", sync None = "block forever") is also why the two parse_response bodies use different pop defaults (None vs SENTINEL), and it's easy to trip over - a short comment on each would help.

Two smaller notes:

• For a positive blocking timeout T, read_response(timeout=T) uses exactly T. Since the server holds the reply until ~T and then sends it, the socket read can expire right as the response arrives (network latency pushes it past T), giving a spurious timeout at the boundary. A small margin (e.g. T + socket_timeout, or a fixed slack) is the usual guard.
• Worth a test for the timeout=0 async case specifically (assert the read is issued with no effective timeout / math.inf), since that's the one most likely to regress.

Happy to be corrected if I'm misreading the async timeout=None semantics, but the docstring reads pretty clearly. Deferring to the maintainers.

[C1-BA-B1-F3]

@eeshsaxena Thanks for the careful read — you are right about the async path.

redis.asyncio.connection.Connection.read_response documents:

• None → fall back to socket_timeout
• math.inf → block indefinitely

So mapping Redis timeout=0 to None on the async client still aborted after socket_timeout, which is exactly the bug for the indefinite-block case. The sync path was fine because there timeout=None means settimeout(None) (block forever).

Pushed a fix in 8f30074:

• async client + async cluster: map _blocking_timeout == 0 → math.inf
• short comments on the sync/async default asymmetry (None vs SENTINEL / meaning of None)
• unit tests asserting parse_response(..., _blocking_timeout=0) calls read_response(timeout=math.inf)

On the positive-timeout margin (T + slack): agreed it is a real boundary race. I left that out of this pass to keep the PR scoped to the indefinite-block correctness issue you flagged; happy to follow up if maintainers want that guard in the same change.

Also noting from earlier discussion: XREAD/XREADGROUP still do not forward _blocking_timeout yet; that is a separate follow-up unless you want it folded in here.

[petyaslavova]

Thanks for the contribution, and for cleaning up the branch — the underlying issue (#2807) is real and forwarding the blocking timeout to the read is a reasonable direction.

However, this can't be merged in its current form because of a correctness regression on the async client.

With a live server, I reproduced the following deterministically on both RESP2 and RESP3 (socket_timeout=1, brpop(timeout=3) on an empty key, max_connections=1): brpop returns None at ~3 s, and the next command on that pooled connection returns a corrupted result (echo("AAA") -> None).
The async read_response(timeout=T) returns None on expiry without disconnecting, and the connection is returned to the pool while the server's blocking reply is still in flight, so it desynchronizes the next command.
On master the same scenario raises TimeoutError and the connection stays clean — so the change currently trades a loud, recoverable error for silent corruption. Using timeout=T with no margin also means the sync path still raises TimeoutError at ~T for the full-block/no-element case.

Could you (1) set the read timeout to the server block timeout plus a margin and ensure a genuine client-side expiry invalidates the connection rather than returning None, and (2) add a sync regression test plus a live-server integration test (socket_timeout < timeout, protocol 2 and 3) that fails on base and passes here? The current tests mock read_response, so they don't exercise this. It's also worth deciding whether BZPOPMIN/MAX, BZMPOP, and XREAD/XREADGROUP block= belong here or as follow-ups, since they share the same root cause.

[eeshsaxena]

Nice fix for the socket_timeout < blocking_timeout case. A question about finite blocking timeouts:

For something like blpop(key, timeout=5) on a connection with socket_timeout=None, this now reads with timeout=5, i.e. the exact blocking timeout. Previously that read blocked until the reply arrived, so it reliably returned None once the server's 5s elapsed. With the read capped at exactly 5s, doesn't it risk timing out at the same instant the server sends the timeout nil, turning a normal "blocked, got None" into a spurious TimeoutError on the RTT boundary?

Would it be worth giving the client a small margin over the server's timeout (e.g. blocking_timeout plus a small buffer) so the socket read always outlives the server-side block? Or is read_response(timeout=...) already lenient enough here that this isn't a concern?