diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index 781239df9c..aad20846fe 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -1,5 +1,6 @@ import asyncio import copy +import errno import inspect import math import socket @@ -344,6 +345,25 @@ def set_parser(self, parser_class: Type[BaseParser]) -> None: """ self._parser = parser_class(socket_read_size=self._socket_read_size) + # OS-level errno values for which reconnecting can never succeed, so + # there is no point spending the retry/backoff budget on them. + _UNRETRYABLE_CONNECT_ERRNOS = frozenset({errno.ENOENT}) + + @classmethod + def _is_retryable_connect_error(cls, error: BaseException) -> bool: + """Whether a failed connection attempt is worth retrying. + + Returns ``False`` for definitively unrecoverable socket errors (e.g. a + Unix socket path that does not exist, ``ENOENT``) so that ``connect()`` + fails fast instead of backing off for every retry. Transient errors + such as ``ECONNREFUSED`` during server startup stay retryable. + """ + cause = error.__cause__ + return not ( + isinstance(cause, OSError) + and cause.errno in cls._UNRETRYABLE_CONNECT_ERRNOS + ) + async def connect(self): """Connects to the Redis server if not already connected""" # try once the socket connect with the handshake, retry the whole @@ -355,6 +375,7 @@ async def connect(self): lambda error, failure_count: self.disconnect( error=error, failure_count=failure_count ), + is_retryable=self._is_retryable_connect_error, with_failure_count=True, ) @@ -395,17 +416,19 @@ def failure_callback(error, failure_count): ) raise e except OSError as e: - e = ConnectionError(self._error_message(e)) + conn_error = ConnectionError(self._error_message(e)) await record_error_count( server_address=getattr(self, "host", None), server_port=getattr(self, "port", None), network_peer_address=getattr(self, "host", None), network_peer_port=getattr(self, "port", None), - error_type=e, + error_type=conn_error, retry_attempts=actual_retry_attempts, is_internal=False, ) - raise e + # Preserve the original OSError (with its errno) as the cause so + # the retry policy can tell unrecoverable failures apart. + raise conn_error from e except Exception as exc: raise ConnectionError(exc) from exc diff --git a/redis/connection.py b/redis/connection.py index c481ec15dc..33f526c3b2 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -1,4 +1,5 @@ import copy +import errno import os import socket import sys @@ -1000,6 +1001,25 @@ def set_parser(self, parser_class): def _get_parser(self) -> Union[_HiredisParser, _RESP3Parser, _RESP2Parser]: return self._parser + # OS-level errno values for which reconnecting can never succeed, so + # there is no point spending the retry/backoff budget on them. + _UNRETRYABLE_CONNECT_ERRNOS = frozenset({errno.ENOENT}) + + @classmethod + def _is_retryable_connect_error(cls, error: BaseException) -> bool: + """Whether a failed connection attempt is worth retrying. + + Returns ``False`` for definitively unrecoverable socket errors (e.g. a + Unix socket path that does not exist, ``ENOENT``) so that ``connect()`` + fails fast instead of backing off for every retry. Transient errors + such as ``ECONNREFUSED`` during server startup stay retryable. + """ + cause = error.__cause__ + return not ( + isinstance(cause, OSError) + and cause.errno in cls._UNRETRYABLE_CONNECT_ERRNOS + ) + def connect(self): "Connects to the Redis server if not already connected" # try once the socket connect with the handshake, retry the whole @@ -1009,6 +1029,7 @@ def connect(self): check_health=True, retry_socket_connect=False ), lambda error: self.disconnect(error), + is_retryable=self._is_retryable_connect_error, ) def connect_check_health( @@ -1044,16 +1065,18 @@ def failure_callback(error, failure_count): ) raise e except OSError as e: - e = ConnectionError(self._error_message(e)) + conn_error = ConnectionError(self._error_message(e)) record_error_count( server_address=getattr(self, "host", None), server_port=getattr(self, "port", None), network_peer_address=getattr(self, "host", None), network_peer_port=getattr(self, "port", None), - error_type=e, + error_type=conn_error, retry_attempts=actual_retry_attempts[0], ) - raise e + # Preserve the original OSError (with its errno) as the cause so + # the retry policy can tell unrecoverable failures apart. + raise conn_error from e self._sock = sock try: diff --git a/tests/test_asyncio/test_connection.py b/tests/test_asyncio/test_connection.py index 2d5557b1d9..f5abdf402d 100644 --- a/tests/test_asyncio/test_connection.py +++ b/tests/test_asyncio/test_connection.py @@ -3,7 +3,7 @@ import ssl import types from unittest import mock -from errno import ECONNREFUSED +from errno import ECONNREFUSED, ENOENT from unittest.mock import patch import pytest @@ -411,6 +411,37 @@ async def test_connect_timeout_error_without_retry(): assert conn._connect.call_count == 1 +@pytest.mark.fixed_client +async def test_connect_without_retry_on_unrecoverable_oserror(): + """A connect error that can never succeed on retry (e.g. a missing Unix + socket path, ENOENT) fails fast instead of consuming the retry budget.""" + conn = Connection(retry=Retry(NoBackoff(), 3)) + conn._connect = mock.AsyncMock() + conn._connect.side_effect = OSError(ENOENT, "No such file or directory") + + with pytest.raises(ConnectionError) as excinfo: + await conn.connect() + # no retries: _connect is called exactly once + assert conn._connect.call_count == 1 + # the original OSError is preserved as the cause for classification + assert isinstance(excinfo.value.__cause__, OSError) + assert excinfo.value.__cause__.errno == ENOENT + + +@pytest.mark.fixed_client +async def test_connect_retries_on_transient_oserror(): + """A transient socket error such as ECONNREFUSED (server not up yet) + stays retryable, so the whole connect flow is retried as before.""" + conn = Connection(retry=Retry(NoBackoff(), 2)) + conn._connect = mock.AsyncMock() + conn._connect.side_effect = OSError(ECONNREFUSED, "Connection refused") + + with pytest.raises(ConnectionError): + await conn.connect() + # 2 retries --> 3 attempts + assert conn._connect.call_count == 3 + + @pytest.mark.onlynoncluster async def test_connection_parse_response_resume(r: redis.Redis): """ diff --git a/tests/test_connection.py b/tests/test_connection.py index 46884bdf86..55b3151eee 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -7,7 +7,7 @@ import ssl import threading import types -from errno import ECONNREFUSED, EWOULDBLOCK +from errno import ECONNREFUSED, ENOENT, EWOULDBLOCK from typing import Any from unittest import mock from unittest.mock import call, patch, MagicMock, Mock @@ -502,6 +502,35 @@ def test_connect_timeout_error_without_retry(self): assert conn._connect.call_count == 1 self.clear(conn) + def test_connect_without_retry_on_unrecoverable_oserror(self): + """A connect error that can never succeed on retry (e.g. a missing Unix + socket path, ENOENT) fails fast instead of consuming the retry budget.""" + conn = Connection(retry=Retry(NoBackoff(), 3)) + conn._connect = mock.Mock() + conn._connect.side_effect = OSError(ENOENT, "No such file or directory") + + with pytest.raises(ConnectionError) as excinfo: + conn.connect() + # no retries: _connect is called exactly once + assert conn._connect.call_count == 1 + # the original OSError is preserved as the cause for classification + assert isinstance(excinfo.value.__cause__, OSError) + assert excinfo.value.__cause__.errno == ENOENT + self.clear(conn) + + def test_connect_retries_on_transient_oserror(self): + """A transient socket error such as ECONNREFUSED (server not up yet) + stays retryable, so the whole connect flow is retried as before.""" + conn = Connection(retry=Retry(NoBackoff(), 2)) + conn._connect = mock.Mock() + conn._connect.side_effect = OSError(ECONNREFUSED, "Connection refused") + + with pytest.raises(ConnectionError): + conn.connect() + # 2 retries --> 3 attempts + assert conn._connect.call_count == 3 + self.clear(conn) + @pytest.mark.onlynoncluster @pytest.mark.parametrize(