Skip to content

Fix bug with partial Hiredis availability #3400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/workflows/integration.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,21 @@ jobs:
invoke ${{matrix.test-type}}-tests
ls -1

- name: Run tests against hiredis < 3.0.0
if: ${{ matrix.connection-type == 'hiredis' && matrix.python-version == '3.12'}}
run: |
pip uninstall hiredis -y
pip install -U setuptools wheel
pip install -r requirements.txt
pip install -r dev_requirements.txt
if [ "${{matrix.connection-type}}" == "hiredis" ]; then
pip install "hiredis<3.0.0"
fi
invoke devenv
sleep 10 # time to settle
invoke ${{matrix.test-type}}-tests
ls -1

- name: Upload test results and profiling data
uses: actions/upload-artifact@v4
with:
Expand Down Expand Up @@ -145,6 +160,24 @@ jobs:
invoke ${{matrix.test-type}}-tests --protocol=3
fi

- name: Run tests against hiredis < 3.0.0
if: ${{ matrix.connection-type == 'hiredis' && matrix.python-version == '3.12'}}
run: |
pip uninstall hiredis -y
pip install -U setuptools wheel
pip install -r requirements.txt
pip install -r dev_requirements.txt
if [ "${{matrix.connection-type}}" == "hiredis" ]; then
pip install "hiredis<3.0.0"
fi
invoke devenv
sleep 10 # time to settle
if [ "${{matrix.event-loop}}" == "uvloop" ]; then
invoke ${{matrix.test-type}}-tests --uvloop --protocol=3
else
invoke ${{matrix.test-type}}-tests --protocol=3
fi

- name: Upload test results and profiling data
uses: actions/upload-artifact@v4
with:
Expand Down
3 changes: 1 addition & 2 deletions redis/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
from .utils import (
CRYPTOGRAPHY_AVAILABLE,
HIREDIS_AVAILABLE,
HIREDIS_PACK_AVAILABLE,
SSL_AVAILABLE,
compare_versions,
ensure_string,
Expand Down Expand Up @@ -314,7 +313,7 @@ def __del__(self):
def _construct_command_packer(self, packer):
if packer is not None:
return packer
elif HIREDIS_PACK_AVAILABLE:
elif HIREDIS_AVAILABLE:
return HiredisRespSerializer()
else:
return PythonRespSerializer(self._buffer_cutoff, self.encoder.encode)
Expand Down
4 changes: 2 additions & 2 deletions redis/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@

# Only support Hiredis >= 3.0:
HIREDIS_AVAILABLE = int(hiredis.__version__.split(".")[0]) >= 3
HIREDIS_PACK_AVAILABLE = hasattr(hiredis, "pack_command")
if not HIREDIS_AVAILABLE:
raise ImportError("hiredis package should be >= 3.0.0")
except ImportError:
HIREDIS_AVAILABLE = False
HIREDIS_PACK_AVAILABLE = False

try:
import ssl # noqa
Expand Down
18 changes: 0 additions & 18 deletions tests/test_encoding.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import pytest
import redis
from redis.connection import Connection
from redis.utils import HIREDIS_PACK_AVAILABLE

from .conftest import _get_client

Expand Down Expand Up @@ -75,22 +73,6 @@ def test_replace(self, request):
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()
arg = memoryview(b"some_arg")
arg_list = ["SOME_COMMAND", arg]
cmd = c.pack_command(*arg_list)
assert cmd[1] is arg
cmds = c.pack_commands([arg_list, arg_list])
assert cmds[1] is arg
assert cmds[3] is arg


class TestCommandsAreNotEncoded:
@pytest.fixture()
def r(self, request):
Expand Down
Loading