Repository: redis/redis-py
Pull request #2570 (2023-02-03 06:31:00)

Use hiredis::pack_command to serialized the commands.
@chayim  @dvora-h
### Pull Request check-list

_Please make sure to review and check all of these items:_

- [x ] Does `$ tox` pass with this change (including linting)?
- [ x] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- [ x] Is the new or changed code fully tested?
- [ ?] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- [n/a ] Is there an example added to the examples folder (if applicable)?
- [ x] Was the change added to CHANGES file?

    Historically, hiredis-py (Python extension module written in C and utilizing hiredis library) has been used only for deserialization of the Redis command's replies. With the new hiredis-py 2.2.1 release it's possible to do the opposite operation - serialization of the Python tuples, representing commands in the redis-py into a RESP-encoded bytes buffer. This PR:
1) Extracts the pack command functionality form the Connection class and delegates it to an external object.
2) PythonRespSerializer class that implements the extracted functionality.
3) HiredisRespSerializer class that uses hiredis-py to serialize a redis-py command.

codecov-commenter: # [Codecov](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) Report
Base: **92.28**% // Head: **78.31**% // Decreases project coverage by **`-13.98%`** :warning:
> Coverage data is based on head [(`566712e`)](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) compared to base [(`428d609`)](https://codecov.io/gh/redis/redis-py/commit/428d60940f386d3680a413aa327889308f82c5de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).
> Patch coverage: 68.00% of modified lines in pull request are covered.

:mega: This organization is not using Codecov’s [GitHub App Integration](https://github.com/apps/codecov). We recommend you install it so Codecov can continue to function properly for your repositories. [Learn more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           master    #2570       +/-   ##
===========================================
- Coverage   92.28%   78.31%   -13.98%     
===========================================
  Files         115      115               
  Lines       29660    29690       +30     
===========================================
- Hits        27373    23252     -4121     
- Misses       2287     6438     +4151     
```


| [Impacted Files](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) | Coverage Δ | |
|---|---|---|
| [redis/utils.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvdXRpbHMucHk=) | `80.35% <50.00%> (-12.24%)` | :arrow_down: |
| [redis/connection.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY29ubmVjdGlvbi5weQ==) | `78.17% <67.39%> (-8.51%)` | :arrow_down: |
| [tests/test\_encoding.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9lbmNvZGluZy5weQ==) | `98.78% <100.00%> (+0.03%)` | :arrow_up: |
| [tests/test\_asyncio/test\_cluster.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9hc3luY2lvL3Rlc3RfY2x1c3Rlci5weQ==) | `18.51% <0.00%> (-79.10%)` | :arrow_down: |
| [tests/test\_cluster.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9jbHVzdGVyLnB5) | `18.00% <0.00%> (-78.87%)` | :arrow_down: |
| [redis/asyncio/cluster.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9jbHVzdGVyLnB5) | `16.96% <0.00%> (-75.05%)` | :arrow_down: |
| [redis/cluster.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY2x1c3Rlci5weQ==) | `18.77% <0.00%> (-71.66%)` | :arrow_down: |
| [redis/asyncio/parser.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9wYXJzZXIucHk=) | `17.30% <0.00%> (-63.47%)` | :arrow_down: |
| [redis/commands/cluster.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY29tbWFuZHMvY2x1c3Rlci5weQ==) | `36.95% <0.00%> (-57.07%)` | :arrow_down: |
| [redis/crc.py](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY3JjLnB5) | `45.45% <0.00%> (-54.55%)` | :arrow_down: |
| ... and [23 more](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/redis/redis-py/pull/2570?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).

chayim: @prokazov given that hiredis isn't a dependency, but is an extra (i.e. ```pip install redis[hiredis]``` we can't enforce a hiredis version on existing users.  This means that the hiredis check now becomes more complicated.

We need to wrap the pack_command call, with a check to ensure that hiredis is >= 2.2.1, specifically due to this change, otherwise we'll break the community.

Similarly @dvora-h this becomes 4.5.0 as discussed.
zalmane: @chayim Originally the check was checking for a version. It was later changed to check for the existence of the pack_command exposed in the hiredis. So effectively this validates if the hiredis-py contains this new method so it is backwards compatible.

Patch:
diff --git a/CHANGES b/CHANGES
index 9d82341a..09babb70 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,4 @@
+    * Use hiredis-py pack_command if available.
     * Support `.unlink()` in ClusterPipeline
     * Simplify synchronous SocketBuffer state management
     * Fix string cleanse in Redis Graph
diff --git a/redis/__init__.py b/redis/__init__.py
index 3ab69706..b8850add 100644
--- a/redis/__init__.py
+++ b/redis/__init__.py
@@ -56,7 +56,7 @@ except metadata.PackageNotFoundError:
 try:
     VERSION = tuple(map(int_or_str, __version__.split(".")))
 except AttributeError:
-    VERSION = tuple(99, 99, 99)
+    VERSION = tuple([99, 99, 99])
 
 __all__ = [
     "AuthenticationError",
diff --git a/redis/connection.py b/redis/connection.py
old mode 100755
new mode 100644
index 57f0a3a8..114221d8
--- a/redis/connection.py
+++ b/redis/connection.py
@@ -3,6 +3,7 @@ import errno
 import io
 import os
 import socket
+import sys
 import threading
 import weakref
 from io import SEEK_END
@@ -32,7 +33,12 @@ from redis.exceptions import (
     TimeoutError,
 )
 from redis.retry import Retry
-from redis.utils import CRYPTOGRAPHY_AVAILABLE, HIREDIS_AVAILABLE, str_if_bytes
+from redis.utils import (
+    CRYPTOGRAPHY_AVAILABLE,
+    HIREDIS_AVAILABLE,
+    HIREDIS_PACK_AVAILABLE,
+    str_if_bytes,
+)
 
 try:
     import ssl
@@ -509,6 +515,75 @@ else:
     DefaultParser = PythonParser
 
 
+class HiredisRespSerializer:
+    def pack(self, *args):
+        """Pack a series of arguments into the Redis protocol"""
+        output = []
+
+        if isinstance(args[0], str):
+            args = tuple(args[0].encode().split()) + args[1:]
+        elif b" " in args[0]:
+            args = tuple(args[0].split()) + args[1:]
+        try:
+            output.append(hiredis.pack_command(args))
+        except TypeError:
+            _, value, traceback = sys.exc_info()
+            raise DataError(value).with_traceback(traceback)
+
+        return output
+
+
+class PythonRespSerializer:
+    def __init__(self, buffer_cutoff, encode) -> None:
+        self._buffer_cutoff = buffer_cutoff
+        self.encode = encode
+
+    def pack(self, *args):
+        """Pack a series of arguments into the Redis protocol"""
+        output = []
+        # the client might have included 1 or more literal arguments in
+        # the command name, e.g., 'CONFIG GET'. The Redis server expects these
+        # arguments to be sent separately, so split the first argument
+        # manually. These arguments should be bytestrings so that they are
+        # not encoded.
+        if isinstance(args[0], str):
+            args = tuple(args[0].encode().split()) + args[1:]
+        elif b" " in args[0]:
+            args = tuple(args[0].split()) + args[1:]
+
+        buff = SYM_EMPTY.join((SYM_STAR, str(len(args)).encode(), SYM_CRLF))
+
+        buffer_cutoff = self._buffer_cutoff
+        for arg in map(self.encode, args):
+            # to avoid large string mallocs, chunk the command into the
+            # output list if we're sending large values or memoryviews
+            arg_length = len(arg)
+            if (
+                len(buff) > buffer_cutoff
+                or arg_length > buffer_cutoff
+                or isinstance(arg, memoryview)
+            ):
+                buff = SYM_EMPTY.join(
+                    (buff, SYM_DOLLAR, str(arg_length).encode(), SYM_CRLF)
+                )
+                output.append(buff)
+                output.append(arg)
+                buff = SYM_CRLF
+            else:
+                buff = SYM_EMPTY.join(
+                    (
+                        buff,
+                        SYM_DOLLAR,
+                        str(arg_length).encode(),
+                        SYM_CRLF,
+                        arg,
+                        SYM_CRLF,
+                    )
+                )
+        output.append(buff)
+        return output
+
+
 class Connection:
     "Manages TCP communication to and from a Redis server"
 
@@ -536,6 +611,7 @@ class Connection:
         retry=None,
         redis_connect_func=None,
         credential_provider: Optional[CredentialProvider] = None,
+        command_packer=None,
     ):
         """
         Initialize a new Connection.
@@ -590,6 +666,7 @@ class Connection:
         self.set_parser(parser_class)
         self._connect_callbacks = []
         self._buffer_cutoff = 6000
+        self._command_packer = self._construct_command_packer(command_packer)
 
     def __repr__(self):
         repr_args = ",".join([f"{k}={v}" for k, v in self.repr_pieces()])
@@ -607,6 +684,14 @@ class Connection:
         except Exception:
             pass
 
+    def _construct_command_packer(self, packer):
+        if packer is not None:
+            return packer
+        elif HIREDIS_PACK_AVAILABLE:
+            return HiredisRespSerializer()
+        else:
+            return PythonRespSerializer(self._buffer_cutoff, self.encoder.encode)
+
     def register_connect_callback(self, callback):
         self._connect_callbacks.append(weakref.WeakMethod(callback))
 
@@ -827,7 +912,8 @@ class Connection:
     def send_command(self, *args, **kwargs):
         """Pack and send a command to the Redis server"""
         self.send_packed_command(
-            self.pack_command(*args), check_health=kwargs.get("check_health", True)
+            self._command_packer.pack(*args),
+            check_health=kwargs.get("check_health", True),
         )
 
     def can_read(self, timeout=0):
@@ -872,48 +958,7 @@ class Connection:
 
     def pack_command(self, *args):
         """Pack a series of arguments into the Redis protocol"""
-        output = []
-        # the client might have included 1 or more literal arguments in
-        # the command name, e.g., 'CONFIG GET'. The Redis server expects these
-        # arguments to be sent separately, so split the first argument
-        # manually. These arguments should be bytestrings so that they are
-        # not encoded.
-        if isinstance(args[0], str):
-            args = tuple(args[0].encode().split()) + args[1:]
-        elif b" " in args[0]:
-            args = tuple(args[0].split()) + args[1:]
-
-        buff = SYM_EMPTY.join((SYM_STAR, str(len(args)).encode(), SYM_CRLF))
-
-        buffer_cutoff = self._buffer_cutoff
-        for arg in map(self.encoder.encode, args):
-            # to avoid large string mallocs, chunk the command into the
-            # output list if we're sending large values or memoryviews
-            arg_length = len(arg)
-            if (
-                len(buff) > buffer_cutoff
-                or arg_length > buffer_cutoff
-                or isinstance(arg, memoryview)
-            ):
-                buff = SYM_EMPTY.join(
-                    (buff, SYM_DOLLAR, str(arg_length).encode(), SYM_CRLF)
-                )
-                output.append(buff)
-                output.append(arg)
-                buff = SYM_CRLF
-            else:
-                buff = SYM_EMPTY.join(
-                    (
-                        buff,
-                        SYM_DOLLAR,
-                        str(arg_length).encode(),
-                        SYM_CRLF,
-                        arg,
-                        SYM_CRLF,
-                    )
-                )
-        output.append(buff)
-        return output
+        return self._command_packer.pack(*args)
 
     def pack_commands(self, commands):
         """Pack multiple commands into the Redis protocol"""
@@ -923,7 +968,7 @@ class Connection:
         buffer_cutoff = self._buffer_cutoff
 
         for cmd in commands:
-            for chunk in self.pack_command(*cmd):
+            for chunk in self._command_packer.pack(*cmd):
                 chunklen = len(chunk)
                 if (
                     buffer_length > buffer_cutoff
diff --git a/redis/utils.py b/redis/utils.py
index 693d4e64..d95e62c0 100644
--- a/redis/utils.py
+++ b/redis/utils.py
@@ -7,8 +7,10 @@ try:
 
     # Only support Hiredis >= 1.0:
     HIREDIS_AVAILABLE = not hiredis.__version__.startswith("0.")
+    HIREDIS_PACK_AVAILABLE = hasattr(hiredis, "pack_command")
 except ImportError:
     HIREDIS_AVAILABLE = False
+    HIREDIS_PACK_AVAILABLE = False
 
 try:
     import cryptography  # noqa


Test patch:
diff --git a/tests/test_encoding.py b/tests/test_encoding.py
index 28676407..cb9c4e20 100644
--- a/tests/test_encoding.py
+++ b/tests/test_encoding.py
@@ -2,6 +2,7 @@ import pytest
 
 import redis
 from redis.connection import Connection
+from redis.utils import HIREDIS_PACK_AVAILABLE
 
 from .conftest import _get_client
 
@@ -75,6 +76,10 @@ class TestEncodingErrors:
         assert r.get("a") == "foo\ufffd"
 
 
+@pytest.mark.skipif(
+    HIREDIS_PACK_AVAILABLE,
+    reason="Packing via hiredis does not preserve memoryviews",
+)
 class TestMemoryviewsAreNotPacked:
     def test_memoryviews_are_not_packed(self):
         c = Connection()