From 71c14782748ca45f26e21ad156844687652ad5e9 Mon Sep 17 00:00:00 2001 From: "Chayim I. Kirshen" Date: Thu, 5 Oct 2023 09:45:23 +0300 Subject: [PATCH 01/28] Python 3.12 --- .github/workflows/integration.yaml | 8 ++++---- setup.py | 5 +++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 7e0fea2e41..e02393c1f0 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -57,7 +57,7 @@ jobs: max-parallel: 15 fail-fast: false matrix: - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', 'pypy-3.7', 'pypy-3.8', 'pypy-3.9'] + python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.7', 'pypy-3.8', 'pypy-3.9'] test-type: ['standalone', 'cluster'] connection-type: ['hiredis', 'plain'] env: @@ -89,7 +89,7 @@ jobs: - name: Upload codecov coverage uses: codecov/codecov-action@v3 - if: ${{matrix.python-version == '3.11'}} + if: ${{matrix.python-version == '3.12'}} with: fail_ci_if_error: false @@ -111,7 +111,7 @@ jobs: strategy: fail-fast: false matrix: - python-version: ['3.7', '3.11'] + python-version: ['3.7', '3.12'] test-type: ['standalone', 'cluster'] connection-type: ['hiredis', 'plain'] protocol: ['3'] @@ -160,7 +160,7 @@ jobs: strategy: fail-fast: false matrix: - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', 'pypy-3.7', 'pypy-3.8', 'pypy-3.9'] + python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.7', 'pypy-3.8', 'pypy-3.9'] steps: - uses: actions/checkout@v3 - uses: actions/setup-python@v4 diff --git a/setup.py b/setup.py index f22d809d27..f1ccedac30 100644 --- a/setup.py +++ b/setup.py @@ -38,7 +38,7 @@ install_requires=[ 'importlib-metadata >= 1.0; python_version < "3.8"', 'typing-extensions; python_version<"3.8"', - 'async-timeout>=4.0.2; python_full_version<="3.11.2"', + 'async-timeout>=4.0.2"', ], classifiers=[ "Development Status :: 5 - Production/Stable", @@ -54,11 +54,12 @@ "Programming Language :: Python :: 3.9", "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", "Programming Language :: Python :: Implementation :: CPython", "Programming Language :: Python :: Implementation :: PyPy", ], extras_require={ "hiredis": ["hiredis>=1.0.0"], - "ocsp": ["cryptography>=36.0.1", "pyopenssl==20.0.1", "requests>=2.26.0"], + "ocsp": ["cryptography>=36.0.1", "pyopenssl==23.2.1", "requests>=2.31.0"], }, ) From 2a81f8aa6ecd57409037fc82ee2b27c72df205a8 Mon Sep 17 00:00:00 2001 From: "Chayim I. Kirshen" Date: Wed, 18 Oct 2023 15:23:34 +0300 Subject: [PATCH 02/28] removed quote --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index f1ccedac30..933c232797 100644 --- a/setup.py +++ b/setup.py @@ -38,7 +38,7 @@ install_requires=[ 'importlib-metadata >= 1.0; python_version < "3.8"', 'typing-extensions; python_version<"3.8"', - 'async-timeout>=4.0.2"', + 'async-timeout>=4.0.2', ], classifiers=[ "Development Status :: 5 - Production/Stable", From 8fe9064619a084930889bc7fb6c17e9b3f7cf516 Mon Sep 17 00:00:00 2001 From: "Chayim I. Kirshen" Date: Wed, 18 Oct 2023 16:15:15 +0300 Subject: [PATCH 03/28] invoke upgrade --- dev_requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev_requirements.txt b/dev_requirements.txt index 3715599af0..0f2d3efe3a 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -8,7 +8,7 @@ packaging>=20.4 pytest==7.2.0 pytest-timeout==2.1.0 pytest-asyncio>=0.20.2 -invoke==1.7.3 +invoke==2.2.0 pytest-cov>=4.0.0 vulture>=2.3.0 ujson>=4.2.0 From be650f43b39cefa57202376e7030785ffd30567c Mon Sep 17 00:00:00 2001 From: "Chayim I. Kirshen" Date: Wed, 10 Jan 2024 10:14:53 +0200 Subject: [PATCH 04/28] ssl context wrapper change for test --- .github/workflows/integration.yaml | 4 ++-- tests/test_connect.py | 18 ++++++++++++++---- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 2f668c081f..0d398da700 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -57,7 +57,7 @@ jobs: max-parallel: 15 fail-fast: false matrix: - python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.7', 'pypy-3.8', 'pypy-3.9'] + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9'] test-type: ['standalone', 'cluster'] connection-type: ['hiredis', 'plain'] env: @@ -159,7 +159,7 @@ jobs: strategy: fail-fast: false matrix: - python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.7', 'pypy-3.8', 'pypy-3.9'] + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9'] steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 diff --git a/tests/test_connect.py b/tests/test_connect.py index 696e69ceea..5812de02c8 100644 --- a/tests/test_connect.py +++ b/tests/test_connect.py @@ -105,13 +105,23 @@ def get_request(self): if self._certfile is None: return super().get_request() newsocket, fromaddr = self.socket.accept() - connstream = ssl.wrap_socket( + context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) + context.load_cert_chain(certfile=self._certfile, keyfile=self._keyfile) + context.options |= ssl.OP_NO_TLSv1_1 + connstream = context.wrap_socket( newsocket, server_side=True, - certfile=self._certfile, - keyfile=self._keyfile, - ssl_version=ssl.PROTOCOL_TLSv1_2, + # certfile=self._certfile, + # keyfile=self._keyfile, + # ssl_version=ssl.PROTOCOL_TLSv1_2, ) + # connstream = ssl.wrap_socket( + # newsocket, + # server_side=True, + # certfile=self._certfile, + # keyfile=self._keyfile, + # ssl_version=ssl.PROTOCOL_TLSv1_2, + # ) return connstream, fromaddr From 9a5b8e8ddb3fd450265eecf483bc3cd91aed6827 Mon Sep 17 00:00:00 2001 From: "Chayim I. Kirshen" Date: Wed, 10 Jan 2024 10:57:02 +0200 Subject: [PATCH 05/28] removing the |= --- tests/test_connect.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/test_connect.py b/tests/test_connect.py index 5812de02c8..9827f8518a 100644 --- a/tests/test_connect.py +++ b/tests/test_connect.py @@ -107,21 +107,10 @@ def get_request(self): newsocket, fromaddr = self.socket.accept() context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) context.load_cert_chain(certfile=self._certfile, keyfile=self._keyfile) - context.options |= ssl.OP_NO_TLSv1_1 connstream = context.wrap_socket( newsocket, server_side=True, - # certfile=self._certfile, - # keyfile=self._keyfile, - # ssl_version=ssl.PROTOCOL_TLSv1_2, ) - # connstream = ssl.wrap_socket( - # newsocket, - # server_side=True, - # certfile=self._certfile, - # keyfile=self._keyfile, - # ssl_version=ssl.PROTOCOL_TLSv1_2, - # ) return connstream, fromaddr From e6487ed943fb6d5d04364d1ecada93d5cec72633 Mon Sep 17 00:00:00 2001 From: "Chayim I. Kirshen" Date: Thu, 11 Jan 2024 09:25:38 +0200 Subject: [PATCH 06/28] seeing how codeql tls silencing works --- tests/test_connect.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_connect.py b/tests/test_connect.py index 9827f8518a..41b4106fd8 100644 --- a/tests/test_connect.py +++ b/tests/test_connect.py @@ -107,6 +107,8 @@ def get_request(self): newsocket, fromaddr = self.socket.accept() context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) context.load_cert_chain(certfile=self._certfile, keyfile=self._keyfile) + context.minimum_version = ssl.TLSVersion.TLSv1_2 + context.maximum_version = ssl.TLSVersion.TLSv1_3 connstream = context.wrap_socket( newsocket, server_side=True, From 1a51cb4b4242bfe5b03f8cb3b568b41ba5a27db7 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 15:51:20 +0300 Subject: [PATCH 07/28] Stabilize more tests and fix linters --- .flake8 | 1 + redis/asyncio/connection.py | 2 +- redis/commands/graph/query_result.py | 17 +++++++++++++- redis/connection.py | 2 +- tests/test_asyncio/test_connect.py | 34 +++++++++------------------- tests/test_connect.py | 26 +++++++++------------ 6 files changed, 41 insertions(+), 41 deletions(-) diff --git a/.flake8 b/.flake8 index d2ee181447..b1bd1d0b75 100644 --- a/.flake8 +++ b/.flake8 @@ -16,6 +16,7 @@ exclude = ignore = E126 E203 + E231 E701 E704 F405 diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index b3dae2a27b..4846ba882c 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -1034,7 +1034,7 @@ def parse_url(url: str) -> ConnectKwargs: try: kwargs[name] = parser(value) except (TypeError, ValueError): - raise ValueError(f"Invalid value for `{name}` in connection URL.") + raise ValueError(f"Invalid value for '{name}' in connection URL.") else: kwargs[name] = value diff --git a/redis/commands/graph/query_result.py b/redis/commands/graph/query_result.py index 7c7f58b99f..7709081bcf 100644 --- a/redis/commands/graph/query_result.py +++ b/redis/commands/graph/query_result.py @@ -1,6 +1,5 @@ import sys from collections import OrderedDict -from distutils.util import strtobool # from prettytable import PrettyTable from redis import ResponseError @@ -571,3 +570,19 @@ async def parse_array(self, value): """ scalar = [await self.parse_scalar(value[i]) for i in range(len(value))] return scalar + + +def strtobool(val): + """ + Convert a string representation of truth to true (1) or false (0). + True values are 'y', 'yes', 't', 'true', 'on', and '1'; false values + are 'n', 'no', 'f', 'false', 'off', and '0'. Raises ValueError if + 'val' is anything else. + """ + val = val.lower() + if val in ("y", "yes", "t", "true", "on", "1"): + return True + elif val in ("n", "no", "f", "false", "off", "0"): + return False + else: + raise ValueError(f"invalid truth value {val!r}") diff --git a/redis/connection.py b/redis/connection.py index 728c221257..3d5407b975 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -992,7 +992,7 @@ def parse_url(url): try: kwargs[name] = parser(value) except (TypeError, ValueError): - raise ValueError(f"Invalid value for `{name}` in connection URL.") + raise ValueError(f"Invalid value for '{name}' in connection URL.") else: kwargs[name] = value diff --git a/tests/test_asyncio/test_connect.py b/tests/test_asyncio/test_connect.py index 0df7ebb43a..943540c885 100644 --- a/tests/test_asyncio/test_connect.py +++ b/tests/test_asyncio/test_connect.py @@ -1,5 +1,4 @@ import asyncio -import logging import re import socket import ssl @@ -14,9 +13,6 @@ from ..ssl_utils import get_ssl_filename -_logger = logging.getLogger(__name__) - - _CLIENT_NAME = "test-suite-client" _CMD_SEP = b"\r\n" _SUCCESS_RESP = b"+OK" + _CMD_SEP @@ -125,7 +121,7 @@ async def test_tcp_ssl_version_mismatch(tcp_address): tcp_address, certfile=certfile, keyfile=keyfile, - ssl_version=ssl.TLSVersion.TLSv1_2, + maximum_ssl_version=ssl.TLSVersion.TLSv1_2, ) await conn.disconnect() @@ -135,7 +131,8 @@ async def _assert_connect( server_address, certfile=None, keyfile=None, - ssl_version=None, + minimum_ssl_version=ssl.TLSVersion.TLSv1_2, + maximum_ssl_version=ssl.TLSVersion.TLSv1_3, ): stop_event = asyncio.Event() finished = asyncio.Event() @@ -153,9 +150,8 @@ async def _handler(reader, writer): elif certfile: host, port = server_address context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) - if ssl_version is not None: - context.minimum_version = ssl_version - context.maximum_version = ssl_version + context.minimum_version = minimum_ssl_version + context.maximum_version = maximum_ssl_version context.load_cert_chain(certfile=certfile, keyfile=keyfile) server = await asyncio.start_server(_handler, host=host, port=port, ssl=context) else: @@ -178,23 +174,18 @@ async def _handler(reader, writer): async def _redis_request_handler(reader, writer, stop_event): - buffer = b"" command = None command_ptr = None fragment_length = None - while not stop_event.is_set() or buffer: - _logger.info(str(stop_event.is_set())) - try: - buffer += await asyncio.wait_for(reader.read(1024), timeout=0.5) - except TimeoutError: - continue + while not stop_event.is_set(): + buffer = await reader.read(1024) if not buffer: - continue + break parts = re.split(_CMD_SEP, buffer) - buffer = parts[-1] - for fragment in parts[:-1]: + for fragment in parts: fragment = fragment.decode() - _logger.info("Command fragment: %s", fragment) + if not fragment: + continue if fragment.startswith("*") and command is None: command = [None for _ in range(int(fragment[1:]))] @@ -214,10 +205,7 @@ async def _redis_request_handler(reader, writer, stop_event): continue command = " ".join(command) - _logger.info("Command %s", command) resp = _SUPPORTED_CMDS.get(command, _ERROR_RESP) - _logger.info("Response from %s", resp) writer.write(resp) await writer.drain() command = None - _logger.info("Exit handler") diff --git a/tests/test_connect.py b/tests/test_connect.py index 534fece3bf..5a2fbf1658 100644 --- a/tests/test_connect.py +++ b/tests/test_connect.py @@ -7,7 +7,7 @@ import pytest from redis.connection import Connection, SSLConnection, UnixDomainSocketConnection -from redis.exceptions import ConnectionError +from redis.exceptions import RedisError from .ssl_utils import get_ssl_filename @@ -126,16 +126,16 @@ def test_tcp_ssl_version_mismatch(tcp_address): port=port, client_name=_CLIENT_NAME, ssl_ca_certs=certfile, - socket_timeout=10, + socket_timeout=3, ssl_min_version=ssl.TLSVersion.TLSv1_3, ) - with pytest.raises(ConnectionError): + with pytest.raises(RedisError): _assert_connect( conn, tcp_address, certfile=certfile, keyfile=keyfile, - ssl_version=ssl.PROTOCOL_TLSv1_2, + maximum_ssl_version=ssl.PROTOCOL_TLSv1_2, ) @@ -164,14 +164,16 @@ def __init__( *args, certfile=None, keyfile=None, - ssl_version=ssl.PROTOCOL_TLS, + minimum_ssl_version=ssl.TLSVersion.TLSv1_2, + maximum_ssl_version=ssl.TLSVersion.TLSv1_3, **kw, ) -> None: self._ready_event = threading.Event() self._stop_requested = False self._certfile = certfile self._keyfile = keyfile - self._ssl_version = ssl_version + self._minimum_ssl_version = minimum_ssl_version + self._maximum_ssl_version = maximum_ssl_version super().__init__(*args, **kw) def service_actions(self): @@ -193,15 +195,9 @@ def get_request(self): newsocket, fromaddr = self.socket.accept() context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH) context.load_cert_chain(certfile=self._certfile, keyfile=self._keyfile) - context.minimum_version = ssl.TLSVersion.TLSv1_2 - context.maximum_version = ssl.TLSVersion.TLSv1_3 - connstream = context.wrap_socket( - newsocket, - server_side=True, - certfile=self._certfile, - keyfile=self._keyfile, - ssl_version=self._ssl_version, - ) + context.minimum_version = self._minimum_ssl_version + context.maximum_version = self._maximum_ssl_version + connstream = context.wrap_socket(newsocket, server_side=True) return connstream, fromaddr From b2a71a93bccf72f5989ee1f15055a419c01c3275 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 17:56:41 +0300 Subject: [PATCH 08/28] Stop requiring typing-extensions --- requirements.txt | 2 +- setup.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index a716b84463..3274a80f62 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1 +1 @@ -async-timeout>=4.0.2 +async-timeout>=4.0.3 diff --git a/setup.py b/setup.py index ac5b59f21c..39cd40b807 100644 --- a/setup.py +++ b/setup.py @@ -36,7 +36,7 @@ author_email="oss@redis.com", python_requires=">=3.8", install_requires=[ - 'async-timeout>=4.0.3; python_full_version<"3.11.3"; typing-extensions;', + 'async-timeout>=4.0.3; python_full_version<"3.11.3"', ], classifiers=[ "Development Status :: 5 - Production/Stable", From f55036de05fcc85dccb8dad1317f2b9504ec61ba Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 18:29:36 +0300 Subject: [PATCH 09/28] Enable tracemalloc --- .github/workflows/integration.yaml | 2 +- tests/conftest.py | 11 +++++++++++ tests/test_connect.py | 2 +- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index ad2fa98457..e66dfee372 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -73,7 +73,7 @@ jobs: with: python-version: ${{ matrix.python-version }} cache: 'pip' - - name: run tests + - name: Run tests run: | pip install -U setuptools wheel pip install -r requirements.txt diff --git a/tests/conftest.py b/tests/conftest.py index e3326f85d5..c24af6cd2d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,7 @@ import argparse import random import time +import tracemalloc from typing import Callable, TypeVar from unittest import mock from unittest.mock import Mock @@ -72,6 +73,16 @@ def format_usage(self): return " | ".join(self.option_strings) +@pytest.fixture(scope="session", autouse=True) +def enable_tracemalloc(): + """ + Enable tracemalloc while tests are being executed. + """ + tracemalloc.start() + yield + tracemalloc.stop() + + def pytest_addoption(parser): parser.addoption( "--redis-url", diff --git a/tests/test_connect.py b/tests/test_connect.py index 5a2fbf1658..a27925a9f4 100644 --- a/tests/test_connect.py +++ b/tests/test_connect.py @@ -135,7 +135,7 @@ def test_tcp_ssl_version_mismatch(tcp_address): tcp_address, certfile=certfile, keyfile=keyfile, - maximum_ssl_version=ssl.PROTOCOL_TLSv1_2, + maximum_ssl_version=ssl.TLSVersion.TLSv1_2, ) From 404405addffb26b5d396b3ef21da771409d7d638 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 18:48:53 +0300 Subject: [PATCH 10/28] Better tracemalloc --- tests/conftest.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index c24af6cd2d..495cf40032 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,6 @@ import argparse import random import time -import tracemalloc from typing import Callable, TypeVar from unittest import mock from unittest.mock import Mock @@ -78,9 +77,14 @@ def enable_tracemalloc(): """ Enable tracemalloc while tests are being executed. """ - tracemalloc.start() - yield - tracemalloc.stop() + try: + import tracemalloc + + tracemalloc.start() + yield + tracemalloc.stop() + except ImportError: + yield def pytest_addoption(parser): From 1634946894d031071e0dc4f6df3cadc76c171cb4 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 19:18:04 +0300 Subject: [PATCH 11/28] Handle possible errors at SSL handshake --- redis/connection.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/redis/connection.py b/redis/connection.py index 3d5407b975..c4b4feafa5 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -834,8 +834,26 @@ def __init__( super().__init__(**kwargs) def _connect(self): - "Wrap the socket with SSL support" + """ + Wrap the socket with SSL support, handling potential errors. + """ sock = super()._connect() + try: + return self._wrap_socket_with_ssl(sock) + except OSError: + sock.close() + raise + + def _wrap_socket_with_ssl(self, sock): + """ + Wraps the socket with SSL support. + + Args: + sock: The plain socket to wrap with SSL. + + Returns: + An SSL wrapped socket. + """ context = ssl.create_default_context() context.check_hostname = self.check_hostname context.verify_mode = self.cert_reqs From fcd9670ee58b39bc2dba608ac5e40041d8584ac5 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 19:43:02 +0300 Subject: [PATCH 12/28] Remove event_loop fixture, as suggested by pytest --- tests/test_asyncio/test_lock.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_asyncio/test_lock.py b/tests/test_asyncio/test_lock.py index c052eae2a0..9eaaed6920 100644 --- a/tests/test_asyncio/test_lock.py +++ b/tests/test_asyncio/test_lock.py @@ -104,16 +104,16 @@ async def test_blocking(self, r): lock_2 = self.get_lock(r, "foo") assert lock_2.blocking - async def test_blocking_timeout(self, r, event_loop): + async def test_blocking_timeout(self, r): lock1 = self.get_lock(r, "foo") assert await lock1.acquire(blocking=False) bt = 0.2 sleep = 0.05 lock2 = self.get_lock(r, "foo", sleep=sleep, blocking_timeout=bt) - start = event_loop.time() + start = asyncio.get_running_loop().time() assert not await lock2.acquire() # The elapsed duration should be less than the total blocking_timeout - assert bt >= (event_loop.time() - start) > bt - sleep + assert bt >= (asyncio.get_running_loop().time() - start) > bt - sleep await lock1.release() async def test_context_manager(self, r): From 35efa8e99dcd8f78063630867577e90c83f49827 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 19:57:07 +0300 Subject: [PATCH 13/28] Fix async tests --- .github/workflows/integration.yaml | 2 +- tests/test_asyncio/test_cwe_404.py | 2 +- tests/test_asyncio/test_search.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index e66dfee372..21f8e488ad 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -129,7 +129,7 @@ jobs: with: python-version: ${{ matrix.python-version }} cache: 'pip' - - name: run tests + - name: Run tests run: | pip install -U setuptools wheel pip install -r requirements.txt diff --git a/tests/test_asyncio/test_cwe_404.py b/tests/test_asyncio/test_cwe_404.py index df46cabc43..e920a3fb98 100644 --- a/tests/test_asyncio/test_cwe_404.py +++ b/tests/test_asyncio/test_cwe_404.py @@ -261,4 +261,4 @@ async def doit(): await asyncio.gather(*[doit() for _ in range(10)]) finally: - await r.close() + await r.aclose() diff --git a/tests/test_asyncio/test_search.py b/tests/test_asyncio/test_search.py index 4cc00c8e5f..9d27a8c7cc 100644 --- a/tests/test_asyncio/test_search.py +++ b/tests/test_asyncio/test_search.py @@ -1511,14 +1511,14 @@ async def test_withsuffixtrie(decoded_r: redis.Redis): # create withsuffixtrie index (text fields) assert await decoded_r.ft().create_index(TextField("t", withsuffixtrie=True)) - waitForIndex(decoded_r, getattr(decoded_r.ft(), "index_name", "idx")) + await waitForIndex(decoded_r, getattr(decoded_r.ft(), "index_name", "idx")) info = await decoded_r.ft().info() assert "WITHSUFFIXTRIE" in info["attributes"][0]["flags"] assert await decoded_r.ft().dropindex("idx") # create withsuffixtrie index (tag field) assert await decoded_r.ft().create_index(TagField("t", withsuffixtrie=True)) - waitForIndex(decoded_r, getattr(decoded_r.ft(), "index_name", "idx")) + await waitForIndex(decoded_r, getattr(decoded_r.ft(), "index_name", "idx")) info = await decoded_r.ft().info() assert "WITHSUFFIXTRIE" in info["attributes"][0]["flags"] From 09201160d313221d1fab10ac0e02eec08878bec9 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Thu, 4 Jul 2024 21:36:11 +0300 Subject: [PATCH 14/28] Fix test results visualization in CI --- .github/workflows/integration.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index ae6e678cf6..08733df4dc 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -159,7 +159,7 @@ jobs: continue-on-error: true with: name: Test Results ${{matrix.python-version}} ${{matrix.test-type}}-${{matrix.connection-type}}-resp3 - path: '*.xml' + path: '*-results.xml' reporter: java-junit list-suites: all list-tests: all From dfa2a9ce0948ea5c91ba6fcf0f4d7dbab8c1b14b Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Fri, 5 Jul 2024 13:53:11 +0300 Subject: [PATCH 15/28] More test fixes Fix checking of module versions. Remove logger from a test, it's just noise in the output. Fix cluster address remap test. --- tests/conftest.py | 4 +++- tests/test_asyncio/test_cluster.py | 13 +++++++++---- tests/test_connect.py | 12 ++---------- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 495cf40032..dd78bb6a2c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -261,7 +261,9 @@ def skip_ifmodversion_lt(min_version: str, module_name: str): for j in modules: if module_name == j.get("name"): version = j.get("ver") - mv = int(min_version.replace(".", "")) + mv = int( + "".join(["%02d" % int(segment) for segment in min_version.split(".")]) + ) check = version < mv return pytest.mark.skipif(check, reason="Redis module version") diff --git a/tests/test_asyncio/test_cluster.py b/tests/test_asyncio/test_cluster.py index 80b0b1bff8..a36040f11b 100644 --- a/tests/test_asyncio/test_cluster.py +++ b/tests/test_asyncio/test_cluster.py @@ -58,7 +58,6 @@ class NodeProxy: def __init__(self, addr, redis_addr): self.addr = addr self.redis_addr = redis_addr - self.send_event = asyncio.Event() self.server = None self.task = None self.n_connections = 0 @@ -83,14 +82,20 @@ async def handle(self, reader, writer): await asyncio.gather(pipe1, pipe2) finally: redis_writer.close() + await self.redis_writer.wait_closed() + writer.close() + await writer.wait_closed() async def aclose(self): - self.task.cancel() try: - await self.task + self.task.cancel() + await asyncio.wait_for(self.task, timeout=1) + self.server.close() + await self.server.wait_closed() + except asyncio.TimeoutError: + pass except asyncio.CancelledError: pass - await self.server.wait_closed() async def pipe( self, diff --git a/tests/test_connect.py b/tests/test_connect.py index a27925a9f4..b11c4446e5 100644 --- a/tests/test_connect.py +++ b/tests/test_connect.py @@ -1,4 +1,3 @@ -import logging import re import socket import socketserver @@ -11,9 +10,6 @@ from .ssl_utils import get_ssl_filename -_logger = logging.getLogger(__name__) - - _CLIENT_NAME = "test-suite-client" _CMD_SEP = b"\r\n" _SUCCESS_RESP = b"+OK" + _CMD_SEP @@ -228,10 +224,10 @@ def is_serving(self): class _RedisRequestHandler(socketserver.StreamRequestHandler): def setup(self): - _logger.info("%s connected", self.client_address) + pass def finish(self): - _logger.info("%s disconnected", self.client_address) + pass def handle(self): buffer = b"" @@ -249,7 +245,6 @@ def handle(self): buffer = parts[-1] for fragment in parts[:-1]: fragment = fragment.decode() - _logger.info("Command fragment: %s", fragment) if fragment.startswith("*") and command is None: command = [None for _ in range(int(fragment[1:]))] @@ -269,9 +264,6 @@ def handle(self): continue command = " ".join(command) - _logger.info("Command %s", command) resp = _SUPPORTED_CMDS.get(command, _ERROR_RESP) - _logger.info("Response %s", resp) self.request.sendall(resp) command = None - _logger.info("Exit handler") From 71d83b3dc2eb7e5bd7dd630e815fd4b0e8776088 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Fri, 5 Jul 2024 15:40:53 +0300 Subject: [PATCH 16/28] Enable test profiling --- .github/workflows/integration.yaml | 8 ++++++-- tasks.py | 8 ++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 08733df4dc..eee080490d 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -143,10 +143,14 @@ jobs: invoke ${{matrix.test-type}}-tests --uvloop --protocol=3 - uses: actions/upload-artifact@v4 - if: success() || failure() with: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 - path: '${{matrix.test-type}}*results.xml' + path: '${{matrix.test-type}}*-results.xml' + + - uses: actions/upload-artifact@v4 + with: + name: pytest-profile-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 + path: '${{matrix.test-type}}-profile.out' - name: Upload codecov coverage uses: codecov/codecov-action@v4 diff --git a/tasks.py b/tasks.py index 7f26081150..07990421d2 100644 --- a/tasks.py +++ b/tasks.py @@ -56,11 +56,11 @@ def standalone_tests(c, uvloop=False, protocol=2): """Run tests against a standalone redis instance""" if uvloop: run( - f"pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml" + f"python -m cProfile -o standalone-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml" ) else: run( - f"pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml" + f"python -m cProfile -o standalone-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml" ) @@ -70,11 +70,11 @@ def cluster_tests(c, uvloop=False, protocol=2): cluster_url = "redis://localhost:16379/0" if uvloop: run( - f"pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop" + f"python -m cProfile -o cluster-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop" ) else: run( - f"pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-results.xml" + f"python -m cProfile -o cluster-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-results.xml" ) From 663e3de303b5f86a5073af2c16e77764d21355fc Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Fri, 5 Jul 2024 15:50:51 +0300 Subject: [PATCH 17/28] Fix upload of tests results --- .github/workflows/integration.yaml | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index eee080490d..a1434e0c51 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -142,15 +142,13 @@ jobs: invoke ${{matrix.test-type}}-tests --protocol=3 invoke ${{matrix.test-type}}-tests --uvloop --protocol=3 - - uses: actions/upload-artifact@v4 + - name: Upload test results and profiling data + uses: actions/upload-artifact@v4 with: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 - path: '${{matrix.test-type}}*-results.xml' - - - uses: actions/upload-artifact@v4 - with: - name: pytest-profile-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 - path: '${{matrix.test-type}}-profile.out' + path: | + '${{matrix.test-type}}*-results.xml' + '${{matrix.test-type}}-profile.out' - name: Upload codecov coverage uses: codecov/codecov-action@v4 From 3668b4334cc3e137e4769ab92f1b44911e4bc4a6 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Fri, 5 Jul 2024 16:07:01 +0300 Subject: [PATCH 18/28] Fix upload of tests results --- .github/workflows/integration.yaml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index a1434e0c51..311c87372b 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -85,11 +85,15 @@ jobs: sleep 10 # time to settle invoke ${{matrix.test-type}}-tests - - uses: actions/upload-artifact@v4 - if: success() || failure() + - name: Upload test results and profiling data + uses: actions/upload-artifact@v4 with: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}} - path: '${{matrix.test-type}}*results.xml' + path: | + '${{matrix.test-type}}*-results.xml' + '${{matrix.test-type}}-profile.out' + if-no-files-found: error + retention-days: 10 - name: Upload codecov coverage uses: codecov/codecov-action@v4 @@ -149,6 +153,8 @@ jobs: path: | '${{matrix.test-type}}*-results.xml' '${{matrix.test-type}}-profile.out' + if-no-files-found: error + retention-days: 10 - name: Upload codecov coverage uses: codecov/codecov-action@v4 From 0e62ac3b809f1e6c88a68dc10130b13e4b8456aa Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Fri, 5 Jul 2024 16:15:36 +0300 Subject: [PATCH 19/28] Fix upload of tests results --- .github/workflows/integration.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 311c87372b..ec33d49574 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -69,10 +69,12 @@ jobs: name: Python ${{ matrix.python-version }} ${{matrix.test-type}}-${{matrix.connection-type}} tests steps: - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} cache: 'pip' + - name: Run tests run: | pip install -U setuptools wheel @@ -84,6 +86,7 @@ jobs: 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 From 6b5f2410fcff344a9e37b6cba6eebb5d2ce4e7a0 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Fri, 5 Jul 2024 16:22:12 +0300 Subject: [PATCH 20/28] Fix upload of tests results --- .github/workflows/integration.yaml | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index ec33d49574..74655fa859 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -93,8 +93,8 @@ jobs: with: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}} path: | - '${{matrix.test-type}}*-results.xml' - '${{matrix.test-type}}-profile.out' + ${{matrix.test-type}}*-results.xml + ${{matrix.test-type}}-profile.out if-no-files-found: error retention-days: 10 @@ -109,7 +109,7 @@ jobs: continue-on-error: true with: name: Test Results ${{matrix.python-version}} ${{matrix.test-type}}-${{matrix.connection-type}} - path: '*.xml' + path: ${{matrix.test-type}}*-results.xml reporter: java-junit list-suites: all list-tests: all @@ -132,10 +132,12 @@ jobs: name: RESP3 [${{ matrix.python-version }} ${{matrix.test-type}}-${{matrix.connection-type}}] steps: - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} cache: 'pip' + - name: Run tests run: | pip install -U setuptools wheel @@ -154,8 +156,8 @@ jobs: with: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 path: | - '${{matrix.test-type}}*-results.xml' - '${{matrix.test-type}}-profile.out' + ${{matrix.test-type}}*-results.xml + ${{matrix.test-type}}-profile.out if-no-files-found: error retention-days: 10 @@ -170,7 +172,7 @@ jobs: continue-on-error: true with: name: Test Results ${{matrix.python-version}} ${{matrix.test-type}}-${{matrix.connection-type}}-resp3 - path: '*-results.xml' + path: ${{matrix.test-type}}*-results.xml reporter: java-junit list-suites: all list-tests: all From a595ff742804ba6f2ac52362432efa26cd152d8a Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 12:05:35 +0300 Subject: [PATCH 21/28] Remove CI test reporting The plugin does not work like this for forked repos. --- .github/workflows/integration.yaml | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 74655fa859..a17851fa4b 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -103,19 +103,6 @@ jobs: with: fail_ci_if_error: false - - name: View Test Results - uses: dorny/test-reporter@v1 - if: success() || failure() - continue-on-error: true - with: - name: Test Results ${{matrix.python-version}} ${{matrix.test-type}}-${{matrix.connection-type}} - path: ${{matrix.test-type}}*-results.xml - reporter: java-junit - list-suites: all - list-tests: all - max-annotations: 10 - fail-on-error: 'false' - resp3_tests: runs-on: ubuntu-latest strategy: @@ -166,19 +153,6 @@ jobs: with: fail_ci_if_error: false - - name: View Test Results - uses: dorny/test-reporter@v1 - if: success() || failure() - continue-on-error: true - with: - name: Test Results ${{matrix.python-version}} ${{matrix.test-type}}-${{matrix.connection-type}}-resp3 - path: ${{matrix.test-type}}*-results.xml - reporter: java-junit - list-suites: all - list-tests: all - max-annotations: 10 - fail-on-error: 'false' - build_and_test_package: name: Validate building and installing the package runs-on: ubuntu-latest From b9ae081e6dfd0af5efa03d370b337195c2bbd768 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 13:52:05 +0300 Subject: [PATCH 22/28] Speed up cluster slot initialization Speed up the computation for slots when initializing a cluster. After profiling, this turned out to be very slow, when it does not have to be. Do profiling a bit differently, via pytest. --- .github/workflows/integration.yaml | 12 +++++----- .gitignore | 2 ++ dev_requirements.txt | 2 ++ redis/cluster.py | 35 ++++++++++++++---------------- tasks.py | 8 +++---- 5 files changed, 31 insertions(+), 28 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index a17851fa4b..15bdf1d18d 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -54,7 +54,7 @@ jobs: pip install -r dev_requirements.txt invoke linters - run-tests: + resp2-tests: runs-on: ubuntu-latest timeout-minutes: 60 strategy: @@ -66,7 +66,7 @@ jobs: connection-type: ['hiredis', 'plain'] env: ACTIONS_ALLOW_UNSECURE_COMMANDS: true - name: Python ${{ matrix.python-version }} ${{matrix.test-type}}-${{matrix.connection-type}} tests + name: RESP2 ${{ matrix.python-version }} ${{matrix.test-type}}-${{matrix.connection-type}} steps: - uses: actions/checkout@v4 @@ -94,7 +94,8 @@ jobs: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}} path: | ${{matrix.test-type}}*-results.xml - ${{matrix.test-type}}-profile.out + prof/** + profile_output* if-no-files-found: error retention-days: 10 @@ -103,7 +104,7 @@ jobs: with: fail_ci_if_error: false - resp3_tests: + resp3-tests: runs-on: ubuntu-latest strategy: fail-fast: false @@ -144,7 +145,8 @@ jobs: name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 path: | ${{matrix.test-type}}*-results.xml - ${{matrix.test-type}}-profile.out + prof/** + profile_output* if-no-files-found: error retention-days: 10 diff --git a/.gitignore b/.gitignore index 3baa34034f..c6a4efd1ec 100644 --- a/.gitignore +++ b/.gitignore @@ -16,4 +16,6 @@ coverage.xml .venv* *.xml .coverage* +prof +profile_output* docker/stunnel/keys diff --git a/dev_requirements.txt b/dev_requirements.txt index 6df1349bc6..1efe6156a1 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -10,6 +10,8 @@ packaging>=20.4 pytest pytest-asyncio pytest-cov +pytest-line-profiler +pytest-profiling pytest-timeout ujson>=4.2.0 urllib3<2 diff --git a/redis/cluster.py b/redis/cluster.py index 144844ec8a..92f7c15eef 100644 --- a/redis/cluster.py +++ b/redis/cluster.py @@ -1522,6 +1522,8 @@ def _get_or_create_cluster_node(self, host, port, role, tmp_nodes_cache): target_node = ClusterNode(host, port, role) if target_node.server_type != role: target_node.server_type = role + # add this node to the nodes cache + tmp_nodes_cache[target_node.name] = target_node return target_node @@ -1585,31 +1587,26 @@ def initialize(self): port = int(primary_node[1]) host, port = self.remap_host_port(host, port) + nodes_for_slot = [] + target_node = self._get_or_create_cluster_node( host, port, PRIMARY, tmp_nodes_cache ) - # add this node to the nodes cache - tmp_nodes_cache[target_node.name] = target_node + nodes_for_slot.append(target_node) + + replica_nodes = [slot[j] for j in range(3, len(slot))] + for replica_node in replica_nodes: + host = str_if_bytes(replica_node[0]) + port = int(replica_node[1]) + host, port = self.remap_host_port(host, port) + target_replica_node = self._get_or_create_cluster_node( + host, port, REPLICA, tmp_nodes_cache + ) + nodes_for_slot.append(target_replica_node) for i in range(int(slot[0]), int(slot[1]) + 1): if i not in tmp_slots: - tmp_slots[i] = [] - tmp_slots[i].append(target_node) - replica_nodes = [slot[j] for j in range(3, len(slot))] - - for replica_node in replica_nodes: - host = str_if_bytes(replica_node[0]) - port = replica_node[1] - host, port = self.remap_host_port(host, port) - - target_replica_node = self._get_or_create_cluster_node( - host, port, REPLICA, tmp_nodes_cache - ) - tmp_slots[i].append(target_replica_node) - # add this node to the nodes cache - tmp_nodes_cache[target_replica_node.name] = ( - target_replica_node - ) + tmp_slots[i] = nodes_for_slot else: # Validate that 2 nodes want to use the same slot cache # setup diff --git a/tasks.py b/tasks.py index 07990421d2..7299a7db1a 100644 --- a/tasks.py +++ b/tasks.py @@ -56,11 +56,11 @@ def standalone_tests(c, uvloop=False, protocol=2): """Run tests against a standalone redis instance""" if uvloop: run( - f"python -m cProfile -o standalone-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml" + f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml" ) else: run( - f"python -m cProfile -o standalone-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml" + f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml" ) @@ -70,11 +70,11 @@ def cluster_tests(c, uvloop=False, protocol=2): cluster_url = "redis://localhost:16379/0" if uvloop: run( - f"python -m cProfile -o cluster-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop" + f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop" ) else: run( - f"python -m cProfile -o cluster-profile.out -m pytest --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-results.xml" + f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-results.xml" ) From 83875973b68de208c5c2d59cf578890c7125b3f5 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 13:55:26 +0300 Subject: [PATCH 23/28] Fix CI --- .github/workflows/integration.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 15bdf1d18d..25fdf12008 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -155,10 +155,10 @@ jobs: with: fail_ci_if_error: false - build_and_test_package: + build-and-test-package: name: Validate building and installing the package runs-on: ubuntu-latest - needs: [run-tests] + needs: [resp2-tests, resp3-tests] strategy: fail-fast: false matrix: @@ -172,7 +172,7 @@ jobs: run: | bash .github/workflows/install_and_test.sh ${{ matrix.extension }} - install_package_from_commit: + install-package-from-commit: name: Install package from commit hash runs-on: ubuntu-latest strategy: From 8b1469d39b1af81aa7eb9548f16d4a014f109efd Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 14:04:32 +0300 Subject: [PATCH 24/28] Remove line profiler, it does not play well with PyPy --- dev_requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/dev_requirements.txt b/dev_requirements.txt index 1efe6156a1..b687bdf1e2 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -10,7 +10,6 @@ packaging>=20.4 pytest pytest-asyncio pytest-cov -pytest-line-profiler pytest-profiling pytest-timeout ujson>=4.2.0 From 9aeec4c28c3388d1e23513312efece1fc30d8f85 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 14:29:09 +0300 Subject: [PATCH 25/28] Make profiling optional --- tasks.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tasks.py b/tasks.py index 7299a7db1a..79b27852e3 100644 --- a/tasks.py +++ b/tasks.py @@ -42,39 +42,39 @@ def all_tests(c): @task -def tests(c, uvloop=False, protocol=2): - """Run the redis-py test suite against the current python, - with and without hiredis. - """ +def tests(c, uvloop=False, protocol=2, profile=False): + """Run the redis-py test suite against the current python.""" print("Starting Redis tests") - standalone_tests(c, uvloop=uvloop, protocol=protocol) - cluster_tests(c, uvloop=uvloop, protocol=protocol) + standalone_tests(c, uvloop=uvloop, protocol=protocol, profile=profile) + cluster_tests(c, uvloop=uvloop, protocol=protocol, profile=profile) @task -def standalone_tests(c, uvloop=False, protocol=2): +def standalone_tests(c, uvloop=False, protocol=2, profile=False): """Run tests against a standalone redis instance""" + profile_arg = "--profile" if profile else "" if uvloop: run( - f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml" + f"pytest {profile_arg} --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml" ) else: run( - f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml" + f"pytest {profile_arg} --protocol={protocol} --cov=./ --cov-report=xml:coverage_redis.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml" ) @task -def cluster_tests(c, uvloop=False, protocol=2): +def cluster_tests(c, uvloop=False, protocol=2, profile=False): """Run tests against a redis cluster""" + profile_arg = "--profile" if profile else "" cluster_url = "redis://localhost:16379/0" if uvloop: run( - f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop" + f"pytest {profile_arg} --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop" ) else: run( - f"pytest --profile --profile-svg --line-profile --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-results.xml" + f"pytest {profile_arg} --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not redismod' --redis-url={cluster_url} --junit-xml=cluster-results.xml" ) From 097938a80322c8a8afb4970cd7199262b3bee09a Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 14:49:25 +0300 Subject: [PATCH 26/28] Improve async cluster connection initialization --- redis/asyncio/cluster.py | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/redis/asyncio/cluster.py b/redis/asyncio/cluster.py index 719c2d2283..e04edccfdd 100644 --- a/redis/asyncio/cluster.py +++ b/redis/asyncio/cluster.py @@ -1317,6 +1317,8 @@ async def initialize(self) -> None: port = int(primary_node[1]) host, port = self.remap_host_port(host, port) + nodes_for_slot = [] + target_node = tmp_nodes_cache.get(get_node_name(host, port)) if not target_node: target_node = ClusterNode( @@ -1324,30 +1326,26 @@ async def initialize(self) -> None: ) # add this node to the nodes cache tmp_nodes_cache[target_node.name] = target_node + nodes_for_slot.append(target_node) + + replica_nodes = [slot[j] for j in range(3, len(slot))] + for replica_node in replica_nodes: + host = replica_node[0] + port = replica_node[1] + host, port = self.remap_host_port(host, port) + + target_replica_node = tmp_nodes_cache.get(get_node_name(host, port)) + if not target_replica_node: + target_replica_node = ClusterNode( + host, port, REPLICA, **self.connection_kwargs + ) + nodes_for_slot.append(target_replica_node) + # add this node to the nodes cache + tmp_nodes_cache[target_replica_node.name] = target_replica_node for i in range(int(slot[0]), int(slot[1]) + 1): if i not in tmp_slots: - tmp_slots[i] = [] - tmp_slots[i].append(target_node) - replica_nodes = [slot[j] for j in range(3, len(slot))] - - for replica_node in replica_nodes: - host = replica_node[0] - port = replica_node[1] - host, port = self.remap_host_port(host, port) - - target_replica_node = tmp_nodes_cache.get( - get_node_name(host, port) - ) - if not target_replica_node: - target_replica_node = ClusterNode( - host, port, REPLICA, **self.connection_kwargs - ) - tmp_slots[i].append(target_replica_node) - # add this node to the nodes cache - tmp_nodes_cache[target_replica_node.name] = ( - target_replica_node - ) + tmp_slots[i] = nodes_for_slot else: # Validate that 2 nodes want to use the same slot cache # setup From f6ed3c595f9445cf46bb7046a3ea688276560a8c Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Mon, 8 Jul 2024 17:07:27 +0300 Subject: [PATCH 27/28] Run uvloop tests in matrix --- .github/workflows/integration.yaml | 12 ++++++++---- redis/asyncio/cluster.py | 4 ++-- redis/cluster.py | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 25fdf12008..ba9e825014 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -112,12 +112,13 @@ jobs: python-version: ['3.8', '3.12'] test-type: ['standalone', 'cluster'] connection-type: ['hiredis', 'plain'] + event-loop: ['asyncio', 'uvloop'] exclude: - test-type: 'cluster' connection-type: 'hiredis' env: ACTIONS_ALLOW_UNSECURE_COMMANDS: true - name: RESP3 [${{ matrix.python-version }} ${{matrix.test-type}}-${{matrix.connection-type}}] + name: RESP3 ${{ matrix.python-version }} ${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.event-loop}} steps: - uses: actions/checkout@v4 @@ -136,13 +137,16 @@ jobs: fi invoke devenv sleep 10 # time to settle - invoke ${{matrix.test-type}}-tests --protocol=3 - invoke ${{matrix.test-type}}-tests --uvloop --protocol=3 + 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: - name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-resp3 + name: pytest-results-${{matrix.test-type}}-${{matrix.connection-type}}-${{matrix.python-version}}-${{matrix.event-loop}}-resp3 path: | ${{matrix.test-type}}*-results.xml prof/** diff --git a/redis/asyncio/cluster.py b/redis/asyncio/cluster.py index e04edccfdd..40b2948a7f 100644 --- a/redis/asyncio/cluster.py +++ b/redis/asyncio/cluster.py @@ -1328,7 +1328,7 @@ async def initialize(self) -> None: tmp_nodes_cache[target_node.name] = target_node nodes_for_slot.append(target_node) - replica_nodes = [slot[j] for j in range(3, len(slot))] + replica_nodes = slot[3:] for replica_node in replica_nodes: host = replica_node[0] port = replica_node[1] @@ -1339,9 +1339,9 @@ async def initialize(self) -> None: target_replica_node = ClusterNode( host, port, REPLICA, **self.connection_kwargs ) - nodes_for_slot.append(target_replica_node) # add this node to the nodes cache tmp_nodes_cache[target_replica_node.name] = target_replica_node + nodes_for_slot.append(target_replica_node) for i in range(int(slot[0]), int(slot[1]) + 1): if i not in tmp_slots: diff --git a/redis/cluster.py b/redis/cluster.py index 92f7c15eef..be7685e9a1 100644 --- a/redis/cluster.py +++ b/redis/cluster.py @@ -1594,7 +1594,7 @@ def initialize(self): ) nodes_for_slot.append(target_node) - replica_nodes = [slot[j] for j in range(3, len(slot))] + replica_nodes = slot[3:] for replica_node in replica_nodes: host = str_if_bytes(replica_node[0]) port = int(replica_node[1]) From bb73758d82d451b1bb740905b2eea3019ab0f660 Mon Sep 17 00:00:00 2001 From: Gabriel Erzse Date: Tue, 9 Jul 2024 11:58:28 +0300 Subject: [PATCH 28/28] Speed up coverage with Python 3.12 Unlock urllib version, to be able to use more recent pytest versions. --- .github/workflows/integration.yaml | 2 ++ dev_requirements.txt | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index ba9e825014..94fe8f35b6 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -25,6 +25,8 @@ permissions: env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + # this speeds up coverage with Python 3.12: https://github.com/nedbat/coveragepy/issues/1665 + COVERAGE_CORE: sysmon REDIS_IMAGE: redis:7.4-rc2 REDIS_STACK_IMAGE: redis/redis-stack-server:7.4.0-rc2 diff --git a/dev_requirements.txt b/dev_requirements.txt index b687bdf1e2..3b0ea8fa03 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -5,7 +5,7 @@ flake8-isort==6.0.0 flake8==5.0.4 flynt~=0.69.0 invoke==2.2.0 -mock==4.0.3 +mock packaging>=20.4 pytest pytest-asyncio @@ -13,7 +13,6 @@ pytest-cov pytest-profiling pytest-timeout ujson>=4.2.0 -urllib3<2 uvloop vulture>=2.3.0 wheel>=0.30.0