fix(asyncio): keep pipeline packing off the event loop - #4210
Conversation
Move async pipeline command packing to a worker thread so large transactions and pipelines do not monopolize the event loop.\n\nRefs redis#3416
petyaslavova
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the underlying issue in #3416 is real: for large async pipelines/transactions, pack_commands runs synchronously on the event loop and can block it for seconds, and moving packing off the loop is a reasonable direction.
There is, though, one design point to settle: the offload is applied unconditionally, so every async pipeline — including small ones — now pays a to_thread dispatch and uses the loop's shared default executor.
It will be better to gate the offload behind a batch-size threshold (so the common small-pipeline path keeps its current cost) or share a quick benchmark showing the small-pipeline overhead is negligible.
It would also help to note whether async cluster pipelines (redis/asyncio/cluster.py, which pack the same way) are in scope here or a follow-up.
Once that's decided and the PR is out of draft, it should be ready for another review.
Summary
Large async pipelines currently call
Connection.pack_commands()synchronously from the event loop. Packing a large transaction or pipeline can therefore block unrelated coroutines for seconds.This change:
asyncio.to_threadFixes #3416.
Validation
pytest -q tests/test_asyncio/test_pipeline.py -k 'pipeline_packing_does_not_block_event_loop or TestAsyncPipelineOperationDurationMetricsRecording'— 7 passedinvoke linters— passedThe full Docker/fixed-client matrix was not completed because the local Docker credential helper and required cluster/maintenance services were unavailable.