Skip to content

Use hiredis command packing for async connections - #4191

Open
sakshichitnis27 wants to merge 4 commits into
redis:masterfrom
sakshichitnis27:issue-4176-async-hiredis-pack-command
Open

Use hiredis command packing for async connections#4191
sakshichitnis27 wants to merge 4 commits into
redis:masterfrom
sakshichitnis27:issue-4176-async-hiredis-pack-command

Conversation

@sakshichitnis27

@sakshichitnis27 sakshichitnis27 commented Jul 15, 2026

Copy link
Copy Markdown

Fixes #4176

Description of change

Async connections currently always use the Python command-packing implementation, even when hiredis.pack_command is available.
This change uses hiredis command packing for async connections when hiredis is installed, while retaining the existing Python fallback. Commands containing memoryviews continue to use the Python path to preserve the existing behavior.
Focused tests cover both hiredis selection and the no-hiredis fallback.

Testing

  • python -m pytest tests/test_asyncio/test_connection.py -q
  • python -m pytest tests/test_asyncio/test_encoding.py -q
  • invoke linters

Pull Request check-list

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the GitHub Actions build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (not applicable; no public API changed)?
  • Is there an example added to the examples folder (not applicable)?

Note

Low Risk
Changes only affect how commands are serialized before send; behavior is aligned with sync hiredis packing with an explicit memoryview fallback and new encoding tests.

Overview
Async Connection.pack_command now routes through the shared HiredisRespSerializer when hiredis is installed, matching sync behavior instead of always using the pure-Python RESP packer. The Python implementation is renamed _pack_command_python and remains the fallback when hiredis is absent or when any argument is a memoryview.

HiredisRespSerializer is refactored to take a fallback packer and the connection’s encoder.encode, so hiredis packing respects connection encoding (e.g. latin-1) rather than coercing arguments with bytes(). Sync _construct_command_packer wires PythonRespSerializer.pack as that fallback.

New tests cover hiredis vs no-hiredis packer selection on async connections and encoding-aware packing for RESP2/3 on both sync and async paths.

Reviewed by Cursor Bugbot for commit 4e9a491. Bugbot is set up for automated code reviews on this repo. Configure here.

@petyaslavova petyaslavova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @sakshichitnis27, thanks for picking this up — closing the async hiredis-packing gap from #4176 is worth doing, and the layering is right. One blocker before we can merge, plus a couple of follow-ups.

Blocker: the hiredis path ignores the connection encoder, so it always encodes str as UTF-8. Async currently honors a custom encoding (e.g. encoding="latin-1" packs café as caf\xe9); with this change and hiredis installed it becomes caf\xc3\xa9. That silently changes the bytes written to Redis for anyone using a non-UTF-8 encoding, so it isn't a no-op / "no public API changed" as the description states. Please preserve the existing behavior — either fall back to the Python packer when encoding isn't the default, or pre-encode the args through the connection encoder before calling hiredis.pack_command — and add a regression test that covers a non-UTF-8 encoding, under both protocols.

Two follow-ups: (1) the new async HiredisRespSerializer diverges from the sync one (fallback-based, different memoryview handling) — let's settle the shared-vs-separate question you raised in the issue and, ideally, share one implementation; (2) the current tests monkeypatch hiredis.pack_command, so they don't verify real wire output — an async equivalent of tests/test_connection.py::test_pack_command would lock it down. Note the sync HiredisRespSerializer has the same encoder gap, so the encoding fix likely belongs in both stacks.

@sakshichitnis27

Copy link
Copy Markdown
Author

@petyaslavova
Thanks for the detailed review. I addressed the blocker and follow-ups in 95eff2f.
Both sync and async now share HiredisRespSerializer.
Arguments are encoded through the connection encoder before calling hiredis.pack_command, preserving custom encodings such as Latin-1.
memoryview arguments retain the Python-packer fallback behavior.
Added real packed-byte regression coverage for non-UTF-8 encoding under RESP2 and RESP3, for both sync and async connections.
Verified the hiredis and pure-Python packing paths; the focused tests and Ruff checks pass.
Could you please take another look?

@eeshsaxena eeshsaxena left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to get hiredis packing onto the async path too. Checked the encoding change: self.encoder is created before _command_packer in __init__, so capturing self.encoder.encode at construction is safe. The meaningful fix is that non-bytearray args now go through self._encode(arg) before hiredis.pack_command, so a connection configured with a non-default encoding (the latin-1 test) packs correctly instead of implicitly assuming UTF-8.

memoryview args are routed to the Python fallback up front, and bytearray still gets bytes(arg), so the zero-copy and buffer cases are preserved. Wiring PythonRespSerializer.pack as the sync fallback keeps parity between the two stacks. Looks correct.

@petyaslavova petyaslavova added maintenance Maintenance (CI, Releases, etc) waiting-for-response labels Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Maintenance (CI, Releases, etc) waiting-for-response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async Redis cannot use hiredis pack_command

3 participants