From a7bab19e1ada40ae3ba4e450a8f29749653232f9 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Thu, 6 Jun 2024 23:57:41 +0900 Subject: [PATCH 01/14] refactor: Improve HELLO command handling with auth args for Redis 6.0 and above, using MULTI command as a fallback --- redis/connection.py | 77 +++++++++++++++++++-------------------------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index f745ecc1d5..e973381f25 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -356,54 +356,41 @@ def on_connect(self): ) auth_args = cred_provider.get_credentials() - # if resp version is specified and we have auth args, - # we need to send them via HELLO - if auth_args and self.protocol not in [2, "2"]: - if isinstance(self._parser, _RESP2Parser): - self.set_parser(_RESP3Parser) - # update cluster exception classes - self._parser.EXCEPTION_CLASSES = parser.EXCEPTION_CLASSES - self._parser.on_connect(self) - if len(auth_args) == 1: - auth_args = ["default", auth_args[0]] - self.send_command("HELLO", self.protocol, "AUTH", *auth_args) - response = self.read_response() - # if response.get(b"proto") != self.protocol and response.get( - # "proto" - # ) != self.protocol: - # raise ConnectionError("Invalid RESP version") - elif auth_args: - # avoid checking health here -- PING will fail if we try - # to check the health prior to the AUTH - self.send_command("AUTH", *auth_args, check_health=False) + auth_command_response = False - try: - auth_response = self.read_response() - except AuthenticationWrongNumberOfArgsError: - # a username and password were specified but the Redis - # server seems to be < 6.0.0 which expects a single password - # arg. retry auth with just the password. - # https://github.com/andymccurdy/redis-py/issues/1274 - self.send_command("AUTH", auth_args[-1], check_health=False) - auth_response = self.read_response() - - if str_if_bytes(auth_response) != "OK": - raise AuthenticationError("Invalid Username or Password") + # try to send HELLO command (for Redis 6.0 and above) + try: + # if resp version is specified and we have auth args, + # we need to send them via HELLO + if auth_args and self.protocol not in [2, "2"]: + if isinstance(self._parser, _RESP2Parser): + self.set_parser(_RESP3Parser) + # update cluster exception classes + self._parser.EXCEPTION_CLASSES = parser.EXCEPTION_CLASSES + self._parser.on_connect(self) + if len(auth_args) == 1: + auth_args = ["default", auth_args[0]] + self.send_command("HELLO", self.protocol, "AUTH", *auth_args) + else: + self.send_command("HELLO", self.protocol) - # if resp version is specified, switch to it - elif self.protocol not in [2, "2"]: - if isinstance(self._parser, _RESP2Parser): - self.set_parser(_RESP3Parser) - # update cluster exception classes - self._parser.EXCEPTION_CLASSES = parser.EXCEPTION_CLASSES - self._parser.on_connect(self) - self.send_command("HELLO", self.protocol) - response = self.read_response() - if ( - response.get(b"proto") != self.protocol - and response.get("proto") != self.protocol - ): + self.read_response() + + except Exception as e: + if str(e) == "Invalid RESP version": raise ConnectionError("Invalid RESP version") + elif str(e) == "Invalid Username or Password": + raise AuthenticationError("Invalid Username or Password") + # fall back to AUTH command (for Redis versions less than 6.0) + else: + self.send_command('MULTI') + self.read_response() + + # avoid checking health here -- PING will fail if we try + # to check the health prior to the AUTH + if auth_args: + self.send_command("AUTH", *auth_args, check_health=False) + auth_command_response = True # if a client_name is given, set it if self.client_name: From 0e9b8cb121a287d58fbfe60b5f7189a7d44708ad Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:00:08 +0900 Subject: [PATCH 02/14] refactor: Add commands for setting client name, library info, database selection, and client caching --- redis/connection.py | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index e973381f25..0f17ab5754 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -395,29 +395,16 @@ def on_connect(self): # if a client_name is given, set it if self.client_name: self.send_command("CLIENT", "SETNAME", self.client_name) - if str_if_bytes(self.read_response()) != "OK": - raise ConnectionError("Error setting client name") - try: - # set the library name and version - if self.lib_name: - self.send_command("CLIENT", "SETINFO", "LIB-NAME", self.lib_name) - self.read_response() - except ResponseError: - pass - - try: - if self.lib_version: - self.send_command("CLIENT", "SETINFO", "LIB-VER", self.lib_version) - self.read_response() - except ResponseError: - pass + # set the library name and version + if self.lib_name: + self.send_command("CLIENT", "SETINFO", "LIB-NAME", self.lib_name) + if self.lib_version: + self.send_command("CLIENT", "SETINFO", "LIB-VER", self.lib_version) # if a database is specified, switch to it if self.db: self.send_command("SELECT", self.db) - if str_if_bytes(self.read_response()) != "OK": - raise ConnectionError("Invalid Database") # if client caching is enabled, start tracking if self.client_cache: From 6909d235172a1bd92e248f97b52c7c9c3a1b11c5 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:01:52 +0900 Subject: [PATCH 03/14] feat: Add handling for MULTI block execution with AUTH response check and error handling --- redis/connection.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/redis/connection.py b/redis/connection.py index 0f17ab5754..4fcb3a1bc9 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -412,6 +412,29 @@ def on_connect(self): self.read_response() self._parser.set_invalidation_push_handler(self._cache_invalidation_process) + # execute the MULTI block + try: + self.send_command('EXEC') + responses = self._read_exec_responses() + # check AUTH response if AUTH command was sent + if auth_command_response: + # First response should be for AUTH command + auth_response = responses[0] + if b'ERR' in auth_response: + raise AuthenticationError( + "Authentication failed: %s" % auth_response) + responses = responses[1:] # Remove AUTH response from the list + + self._handle_responses(responses, auth_args) + except (TimeoutError, AuthenticationError, ConnectionError) as e: + if not self.retry_on_timeout: + raise e + except Exception as e: + if str(e) == "Invalid Username or Password": + raise AuthenticationError("Invalid Username or Password") from e + else: + raise AuthenticationError() from e + def disconnect(self, *args): "Disconnects from the Redis server" self._parser.on_disconnect() From 72292f91566490100da8218db87d1e9a31193a4c Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:05:40 +0900 Subject: [PATCH 04/14] feat: Add error handling for EXEC command response with retry logic consideration --- redis/connection.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/redis/connection.py b/redis/connection.py index 4fcb3a1bc9..cad6831505 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -435,6 +435,19 @@ def on_connect(self): else: raise AuthenticationError() from e + def _read_exec_responses(self): + # read the response for EXEC which should be a list + response = self.read_response() + if response == b'OK' and not self.retry_on_timeout: + # EXEC did not execute correctly, likely due to previous error + raise ConnectionError("EXEC command did not execute correctly") + while response == b'QUEUED': + response = self.read_response() + if not isinstance(response, list) and not self.retry_on_timeout: + raise ConnectionError(f"EXEC command did not return a list: {response}") + return response + + def disconnect(self, *args): "Disconnects from the Redis server" self._parser.on_disconnect() From 0cd59b85958f68db7a9aafab07149c269cc89a35 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:07:00 +0900 Subject: [PATCH 05/14] feat: add response handling for various client commands with detailed error checks --- redis/connection.py | 73 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index cad6831505..05d5c90197 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -409,8 +409,6 @@ def on_connect(self): # if client caching is enabled, start tracking if self.client_cache: self.send_command("CLIENT", "TRACKING", "ON") - self.read_response() - self._parser.set_invalidation_push_handler(self._cache_invalidation_process) # execute the MULTI block try: @@ -447,6 +445,77 @@ def _read_exec_responses(self): raise ConnectionError(f"EXEC command did not return a list: {response}") return response + def _handle_responses(self, responses, auth_args): + if not isinstance(responses, list): + raise ConnectionError(f"EXEC command did not return a list: {responses}") + + response_iter = iter(responses) + + try: + # handle HELLO + AUTH + if auth_args and self.protocol not in [2, "2"]: + response = next(response_iter, None) + if isinstance(response, dict) and ( + response.get(b"proto") != self.protocol and response.get("proto") != self.protocol): + raise ConnectionError("Invalid RESP version") + + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + raise AuthenticationError("Invalid Username or Password") + elif auth_args: + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + try: + # a username and password were specified but the Redis + # server seems to be < 6.0.0 which expects a single password + # arg. retry auth with just the password. + # https://github.com/andymccurdy/redis-py/issues/1274 + self.send_command("AUTH", auth_args[-1], check_health=False) + auth_response = self.read_response() + if isinstance(auth_response, bytes) and str_if_bytes( + auth_response) != "OK": + raise AuthenticationError("Invalid Username or Password") + # add the retry response to the responses list for further processing + responses = [auth_response] + list(response_iter) + response_iter = iter(responses) + except AuthenticationWrongNumberOfArgsError: + raise AuthenticationError("Invalid Username or Password") + + # handle CLIENT SETNAME + if self.client_name: + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + raise ConnectionError("Error setting client name") + + # handle CLIENT SETINFO LIB-NAME + if self.lib_name: + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + raise ConnectionError("Error setting client library name") + + # handle CLIENT SETINFO LIB-VER + if self.lib_version: + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + raise ConnectionError("Error setting client library version") + + # handle SELECT + if self.db: + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + raise ConnectionError("Invalid Database") + + # handle CLIENT TRACKING ON + if self.client_cache: + response = next(response_iter, None) + if isinstance(response, bytes) and str_if_bytes(response) != "OK": + raise ConnectionError("Error enabling client tracking") + self._parser.set_invalidation_push_handler( + self._cache_invalidation_process) + except (AuthenticationError, ConnectionError): + raise + except Exception as e: + raise ConnectionError("Error during response handling") from e def disconnect(self, *args): "Disconnects from the Redis server" From 3a6cb1e6eff15d0ba9f0c67ba3a7e32f6720aaf1 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:09:54 +0900 Subject: [PATCH 06/14] test: test for on_connect to verify correct command sequence and responses connection --- tests/test_connection.py | 53 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index bff249559e..fc55293115 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -1,8 +1,9 @@ +import itertools import socket import types +from unittest import TestCase from unittest import mock -from unittest.mock import patch - +from unittest.mock import patch, MagicMock import pytest import redis from redis import ConnectionPool, Redis @@ -13,6 +14,8 @@ SSLConnection, UnixDomainSocketConnection, parse_url, + UsernamePasswordCredentialProvider, + AuthenticationError ) from redis.exceptions import ConnectionError, InvalidResponse, TimeoutError from redis.retry import Retry @@ -55,7 +58,7 @@ def inner(): # assert mod.get('fookey') == d -class TestConnection: +class TestConnection(TestCase): def test_disconnect(self): conn = Connection() mock_sock = mock.Mock() @@ -131,6 +134,50 @@ def test_connect_timeout_error_without_retry(self): assert str(e.value) == "Timeout connecting to server" self.clear(conn) + @patch.object(Connection, 'send_command') + @patch.object(Connection, 'read_response') + def test_on_connect(self, mock_read_response, mock_send_command): + """Test that the on_connect function sends the correct commands""" + conn = Connection() + + conn._parser = MagicMock() + conn._parser.on_connect.return_value = None + conn.credential_provider = None + conn.username = "myuser" + conn.password = "password" + conn.protocol = 3 + conn.client_name = "test-client" + conn.lib_name = "test" + conn.lib_version = "1234" + conn.db = 0 + conn.client_cache = True + + # command response + mock_read_response.side_effect = itertools.cycle([ + b'QUEUED', # MULTI + b'QUEUED', # HELLO + b'QUEUED', # AUTH + b'QUEUED', # CLIENT SETNAME + b'QUEUED', # CLIENT SETINFO LIB-NAME + b'QUEUED', # CLIENT SETINFO LIB-VER + b'QUEUED', # SELECT + b'QUEUED', # CLIENT TRACKING ON + [ # EXEC response list + {"proto": 3, "version": "6"}, + b'OK', + b'OK', + b'OK', + b'OK', + b'OK', + b'OK', + b'OK' + ] + ]) + + conn.on_connect() + + mock_read_response.side_effect = itertools.repeat("OK") + @pytest.mark.onlynoncluster @pytest.mark.parametrize( From 7754937a14222eb4751d6e676c8239947f394065 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:21:13 +0900 Subject: [PATCH 07/14] test: test for on_connect to handle HELLO command failure --- tests/test_connection.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index fc55293115..04826c6b5c 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -178,6 +178,39 @@ def test_on_connect(self, mock_read_response, mock_send_command): mock_read_response.side_effect = itertools.repeat("OK") + @patch.object(Connection, 'send_command') + @patch.object(Connection, 'read_response') + def test_on_connect_fail_hello(self, mock_read_response, mock_send_command): + """Test that on_connect handles connection failure HELLO command""" + conn = Connection() + + conn._parser = MagicMock() + conn._parser.on_connect.return_value = None + conn.credential_provider = None + conn.username = "myuser" + conn.password = "password" + conn.protocol = -1 # invalid protocol + conn.client_name = "test-client" + conn.lib_name = "test" + conn.lib_version = "1234" + conn.db = 0 + conn.client_cache = True + + # simulate a failure in the HELLO command response + mock_read_response.side_effect = itertools.cycle([ + Exception("Invalid RESP version"), # HELLO (fails) + b'QUEUED', # MULTI + ]) + + with self.assertRaises(ConnectionError): + conn.on_connect() + + mock_send_command.assert_any_call('HELLO', -1, 'AUTH', 'myuser', 'password'), + + mock_send_command.assert_called() + mock_read_response.assert_called() + + @pytest.mark.onlynoncluster @pytest.mark.parametrize( From 2121304054b811e6859b15d1fc1cee0379478a7d Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:24:22 +0900 Subject: [PATCH 08/14] test: test for on_connect to handle AUTH failure --- tests/test_connection.py | 54 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index 04826c6b5c..473d184930 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -210,6 +210,60 @@ def test_on_connect_fail_hello(self, mock_read_response, mock_send_command): mock_send_command.assert_called() mock_read_response.assert_called() + @patch.object(Connection, 'send_command') + @patch.object(Connection, 'read_response') + def test_on_connect_fail_auth(self, mock_read_response, mock_send_command): + """Test that on_connect handles connection failure AUTH command""" + conn = Connection() + + conn._parser = MagicMock() + conn._parser.on_connect.return_value = None + conn.credential_provider = None + conn.username = "myuser" + conn.password = "wrong-password" + conn.protocol = 3 + conn.client_name = "test-client" + conn.lib_name = "test" + conn.lib_version = "1234" + conn.db = 1 + conn.client_cache = True + + # simulate a failure in the HELLO command response + mock_read_response.side_effect = itertools.cycle([ + {"proto": 3, "version": "6"}, # HELLO + b'QUEUED', # MULTI + b'QUEUED', # AUTH + b'QUEUED', # CLIENT SETNAME + b'QUEUED', # CLIENT SETINFO LIB-NAME + b'QUEUED', # CLIENT SETINFO LIB-VER + b'QUEUED', # SELECT + b'QUEUED', # CLIENT TRACKING ON + [ + {"proto": 3, "version": "6"}, # HELLO response + b'ERR invalid password', # AUTH response + b'OK', # CLIENT SETNAME response + b'OK', # CLIENT SETINFO LIB-NAME response + b'OK', # CLIENT SETINFO LIB-VER response + b'OK', # SELECT response + b'OK' # CLIENT TRACKING ON response + ] + ]) + + with self.assertRaises(AuthenticationError): + conn.on_connect() + + mock_send_command.assert_any_call( + 'HELLO', 3, 'AUTH', 'myuser', 'wrong-password'), + mock_send_command.assert_any_call('CLIENT', 'SETNAME', 'test-client'), + mock_send_command.assert_any_call('CLIENT', 'SETINFO', 'LIB-NAME', 'test'), + mock_send_command.assert_any_call('CLIENT', 'SETINFO', 'LIB-VER', '1234'), + mock_send_command.assert_any_call('SELECT', 1), + mock_send_command.assert_any_call('CLIENT', 'TRACKING', 'ON'), + mock_send_command.assert_any_call('EXEC') + + mock_send_command.assert_called() + mock_read_response.assert_called() + @pytest.mark.onlynoncluster From b16868f13c309573e9258b266495ee6cebfcb854 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Fri, 7 Jun 2024 00:30:17 +0900 Subject: [PATCH 09/14] test: Test password-only AUTH for Redis versions below 6.0.0 without HELLO command --- tests/test_connection.py | 52 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/test_connection.py b/tests/test_connection.py index 473d184930..a724c2e5b9 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -264,6 +264,58 @@ def test_on_connect_fail_auth(self, mock_read_response, mock_send_command): mock_send_command.assert_called() mock_read_response.assert_called() + @patch.object(Connection, 'send_command') + @patch.object(Connection, 'read_response') + def test_on_connect_auth_with_password_only( + self, mock_read_response, mock_send_command): + """Test on_connect handling of password-only AUTH for Redis versions below 6.0.0 without HELLO command""" + conn = Connection() + + conn._parser = MagicMock() + conn._parser.on_connect.return_value = None + conn.credential_provider = None + conn.username = None + conn.password = "password" + conn.protocol = 1 + conn.client_name = "test-client" + conn.lib_name = "test" + conn.lib_version = "1234" + conn.db = 1 + conn.client_cache = True + + # command response to simulate Redis < 6.0.0 behavior + mock_read_response.side_effect = itertools.cycle([ + Exception("ERR HELLO"), # HELLO (fails) + b'QUEUED', # MULTI + b'QUEUED', # AUTH + b'QUEUED', # CLIENT SETNAME + b'QUEUED', # CLIENT SETINFO LIB-NAME + b'QUEUED', # CLIENT SETINFO LIB-VER + b'QUEUED', # SELECT + b'QUEUED', # CLIENT TRACKING ON + [ + b'OK', # AUTH response + b'OK', # CLIENT SETNAME response + b'OK', # CLIENT SETINFO LIB-NAME response + b'OK', # CLIENT SETINFO LIB-VER response + b'OK', # SELECT response + b'OK' # CLIENT TRACKING ON response + ] + ]) + + conn.on_connect() + + mock_send_command.assert_any_call('HELLO', 1, 'AUTH', 'default', 'password'), + mock_send_command.assert_any_call('MULTI'), + mock_send_command.assert_any_call( + 'AUTH', 'default', 'password', check_health=False) + mock_send_command.assert_any_call('CLIENT', 'SETNAME', 'test-client') + mock_send_command.assert_any_call('CLIENT', 'SETINFO', 'LIB-NAME', 'test') + mock_send_command.assert_any_call('CLIENT', 'SETINFO', 'LIB-VER', '1234') + mock_send_command.assert_any_call('SELECT', 1) + mock_send_command.assert_any_call('CLIENT', 'TRACKING', 'ON') + mock_send_command.assert_any_call('EXEC') + mock_read_response.assert_called() @pytest.mark.onlynoncluster From 241e1197777440227cedccb3c97d95755a70380e Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Tue, 11 Jun 2024 23:34:06 +0900 Subject: [PATCH 10/14] refactor: Add AUTH command to send_command based on Redis version check if no exceptions occur. --- redis/connection.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index 05d5c90197..392369e4b6 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -356,8 +356,6 @@ def on_connect(self): ) auth_args = cred_provider.get_credentials() - auth_command_response = False - # try to send HELLO command (for Redis 6.0 and above) try: # if resp version is specified and we have auth args, @@ -379,8 +377,6 @@ def on_connect(self): except Exception as e: if str(e) == "Invalid RESP version": raise ConnectionError("Invalid RESP version") - elif str(e) == "Invalid Username or Password": - raise AuthenticationError("Invalid Username or Password") # fall back to AUTH command (for Redis versions less than 6.0) else: self.send_command('MULTI') @@ -391,6 +387,15 @@ def on_connect(self): if auth_args: self.send_command("AUTH", *auth_args, check_health=False) auth_command_response = True + # avoid checking health here -- PING will fail if we try + # to check the health prior to the AUTH + if auth_args: + # check if only password is provided and RESP version < 6 + if not self.username and self.password and self.protocol in [2, "2"]: + self.send_command("AUTH", self.password, check_health=False) + else: + self.send_command("AUTH", *auth_args, check_health=False) + # if a client_name is given, set it if self.client_name: From e6af1f426122c809c9647df083a9bf3f11938d1d Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Tue, 11 Jun 2024 23:40:26 +0900 Subject: [PATCH 11/14] refactor: MULTI and subsequent commands --- redis/connection.py | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index 392369e4b6..5120516c50 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -379,14 +379,6 @@ def on_connect(self): raise ConnectionError("Invalid RESP version") # fall back to AUTH command (for Redis versions less than 6.0) else: - self.send_command('MULTI') - self.read_response() - - # avoid checking health here -- PING will fail if we try - # to check the health prior to the AUTH - if auth_args: - self.send_command("AUTH", *auth_args, check_health=False) - auth_command_response = True # avoid checking health here -- PING will fail if we try # to check the health prior to the AUTH if auth_args: @@ -396,24 +388,28 @@ def on_connect(self): else: self.send_command("AUTH", *auth_args, check_health=False) + # start a transaction block with MULTI + try: + self.send_command('MULTI') + self.read_response() - # if a client_name is given, set it - if self.client_name: - self.send_command("CLIENT", "SETNAME", self.client_name) + # if a client_name is given, set it + if self.client_name: + self.send_command("CLIENT", "SETNAME", self.client_name) - # set the library name and version - if self.lib_name: - self.send_command("CLIENT", "SETINFO", "LIB-NAME", self.lib_name) - if self.lib_version: - self.send_command("CLIENT", "SETINFO", "LIB-VER", self.lib_version) + # set the library name and version + if self.lib_name: + self.send_command("CLIENT", "SETINFO", "LIB-NAME", self.lib_name) + if self.lib_version: + self.send_command("CLIENT", "SETINFO", "LIB-VER", self.lib_version) - # if a database is specified, switch to it - if self.db: - self.send_command("SELECT", self.db) + # if a database is specified, switch to it + if self.db: + self.send_command("SELECT", self.db) - # if client caching is enabled, start tracking - if self.client_cache: - self.send_command("CLIENT", "TRACKING", "ON") + # if client caching is enabled, start tracking + if self.client_cache: + self.send_command("CLIENT", "TRACKING", "ON") # execute the MULTI block try: From 1d4c67953a6164cb7d3625a592e38a8450370a6e Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Tue, 11 Jun 2024 23:44:52 +0900 Subject: [PATCH 12/14] refactor: Modify code to execute the EXEC command within the try block where the MULTI command is executed. --- redis/connection.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index 5120516c50..c5f08e9e3c 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -411,8 +411,7 @@ def on_connect(self): if self.client_cache: self.send_command("CLIENT", "TRACKING", "ON") - # execute the MULTI block - try: + # execute the MULTI block self.send_command('EXEC') responses = self._read_exec_responses() # check AUTH response if AUTH command was sent From eae6d46262a4ee4a324be7c23ed0f0924a081b9e Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Tue, 11 Jun 2024 23:49:07 +0900 Subject: [PATCH 13/14] fix: Remove redundant AUTH command check logic as similar logic exists (lines 461-468). --- redis/connection.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index c5f08e9e3c..94ce151bc9 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -414,15 +414,6 @@ def on_connect(self): # execute the MULTI block self.send_command('EXEC') responses = self._read_exec_responses() - # check AUTH response if AUTH command was sent - if auth_command_response: - # First response should be for AUTH command - auth_response = responses[0] - if b'ERR' in auth_response: - raise AuthenticationError( - "Authentication failed: %s" % auth_response) - responses = responses[1:] # Remove AUTH response from the list - self._handle_responses(responses, auth_args) except (TimeoutError, AuthenticationError, ConnectionError) as e: if not self.retry_on_timeout: From c4c21d06f7930e0110e617c8be5fcbdf8d776c47 Mon Sep 17 00:00:00 2001 From: "sojung.kim" Date: Tue, 11 Jun 2024 23:55:46 +0900 Subject: [PATCH 14/14] fix:Remove the condition that checks for connection retries This update eliminates the condition checking for connection retries because the logic was refactored to execute MULTI and EXEC commands within the same try block. This adjustment resolves the issue with unintended reconnection attempts, ensuring proper handling of transactional commands --- redis/connection.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index 94ce151bc9..5e9edb4e70 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -415,24 +415,21 @@ def on_connect(self): self.send_command('EXEC') responses = self._read_exec_responses() self._handle_responses(responses, auth_args) - except (TimeoutError, AuthenticationError, ConnectionError) as e: - if not self.retry_on_timeout: - raise e - except Exception as e: + except AuthenticationError as e: if str(e) == "Invalid Username or Password": raise AuthenticationError("Invalid Username or Password") from e - else: - raise AuthenticationError() from e + except Exception: + raise ConnectionError("Error during EXEC handling") def _read_exec_responses(self): # read the response for EXEC which should be a list response = self.read_response() - if response == b'OK' and not self.retry_on_timeout: + if response == b'OK': # EXEC did not execute correctly, likely due to previous error raise ConnectionError("EXEC command did not execute correctly") while response == b'QUEUED': response = self.read_response() - if not isinstance(response, list) and not self.retry_on_timeout: + if not isinstance(response, list): raise ConnectionError(f"EXEC command did not return a list: {response}") return response