fix(worker): [python] isolate blocking client when sharing Redis connection - #4103
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a Python-only bug where sharing the same redis.asyncio.Redis instance between Queue and Worker can corrupt replies when the worker issues blocking commands, leading to empty job.data / None job.name.
Changes:
- Add
shared+isBlockingbehavior toRedisConnectionto treat externally-supplied clients as caller-owned and to derive a dedicated blocking client. - Update
Workerto create its blocking connection withisBlocking=True. - Add a regression test suite covering shared-client behavior and blocking-client isolation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/bullmq/redis_connection.py | Track externally-owned clients and attempt to isolate blocking usage via a derived sibling client. |
| python/bullmq/worker.py | Use isBlocking=True for the worker’s blocking Redis connection. |
| python/tests/shared_connection_test.py | Add regression tests for shared client + blocking-client isolation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the review @manast! Pushed a follow-up commit addressing every point:
All seven shared-connection tests pass locally. Could you take another look when you have a moment? Happy to iterate on anything that still feels off. |
…ection (taskforcesh#3401) When a user passes the same redis.asyncio.Redis instance to both Queue and Worker, the worker's blocking client (used for BZPOPMIN) ended up reusing the same underlying socket as the regular command client because RedisConnection(redisOpts) was called twice with the same already-constructed Redis instance. Blocking commands hold the socket for extended periods, interleaving with and corrupting replies of subsequent non-blocking commands. The user-visible symptom was job.data arriving as an empty dict and job.name as None inside the worker processor. The fix mirrors the Node implementation's behaviour: - RedisConnection now tracks a "shared" flag whenever the caller supplies an already-constructed Redis instance, and close() / disconnect() are no-ops in that case so the caller retains ownership of their client. - RedisConnection gained an isBlocking flag. When it is True and the caller supplied a Redis instance, we derive a sibling client via Redis.client() so the blocking operations run on a dedicated connection from the same pool. - Worker constructs its blockingRedisConnection with isBlocking=True so BZPOPMIN can no longer starve the regular client's replies. Added tests/shared_connection_test.py with six regression tests covering the shared flag, close/disconnect being no-ops, reuse of the caller client for non-blocking paths, duplication for blocking paths, and an end-to-end Worker-level assertion that worker.client and worker.bclient are distinct instances when a shared client is passed.
…tion - Trim long inline comments in redis_connection.py and worker.py per maintainer feedback. - Reword the close() docstring to describe behaviour rather than prescribe what we must not do. - Only mark the connection as shared when it is the literal caller- supplied client; the derived blocking sibling is owned by us and must be released in close()/disconnect(). Previously the sibling inherited shared=True, so worker.close() would leak the sibling for the lifetime of the caller's client. - Drop the issue-specific framing in the test docstring and class name, consolidate overlapping tests, and assert that worker.close() releases the derived blocking client without touching the caller's shared client.
8e6d334 to
9f3c444
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ection Cover the original failure mode end-to-end: build a single redis.asyncio.Redis client, hand it to both Queue and Worker, add a job, and assert the processor sees the correct job.name and job.data — the exact symptoms reported in taskforcesh#3401 when the blocking client and regular client shared a socket. Also assert the caller's shared client is still usable after worker.close().
|
Thanks for the suggestion! Pushed a follow-up commit (
The 7 mock-based unit tests continue to pass locally; the new integration test will run against the |
Fixes #3401
Why
When a user passes the same
redis.asyncio.Redisinstance to bothQueueandWorker,job.dataarrives as an empty dict andjob.nameasNoneinside the processor. Using Redis URL strings works fine.Since #3887 unified Python connections on
single_connection_client=True,Worker.__init__callsRedisConnection(redis_opts)twice — once for the regular client and once for the blocking client. Whenredis_optsis an already-constructedredis.Redis,RedisConnectionreused that same instance verbatim for both connections, so the Worker'sBZPOPMINblocking command monopolised the single underlying socket and interleaved with / corrupted replies of subsequent commands (theHMGETthat reads the job hash aftermoveToActive). Node avoids this by calling.duplicate()on externally-supplied clients (src/classes/worker.ts) and creating the blockingRedisConnectionwithshared: false(src/classes/redis-connection.ts).How
RedisConnectionnow tracks asharedflag whenever the caller supplies an already-constructed Redis instance;close()anddisconnect()are no-ops in that case so the caller retains ownership of their client.RedisConnectiongained anisBlockingflag. WhenisBlocking=Trueand the caller supplied a Redis instance, the constructor derives a sibling client viaRedis.client()so blocking operations run on a dedicated connection from the same pool.Workerconstructs itsblockingRedisConnectionwithisBlocking=TruesoBZPOPMINcan no longer starve the regular client's replies.Additional Notes (Optional)
python/tests/shared_connection_test.py— 6 mock-based regression tests covering thesharedflag, close/disconnect being no-ops, reuse of the caller's client on the non-blocking path, duplication on the blocking path, and an end-to-end assertion thatworker.client is not worker.bclientwhen a shared client is passed.flake8 --select=E9,F63,F7,F82 bullmq testsis clean; existing non-integration tests still pass.[python].