Skip to content

Commit 0cd0c5d

Browse files
gerzsegerzse
committed
Format connection errors in the same way everywhere (#3305)
Connection errors are formatted in four places, sync and async, network socket and unix socket. Each place has some small differences compared to the others, while they could be, and should be, formatted in an uniform way. Factor out the logic in a helper method and call that method in all four places. Arguably we lose some specificity, e.g. the words "unix socket" won't be there anymore, but it is more valuable to not have code duplication.
1 parent 6fedfef commit 0cd0c5d

File tree

5 files changed

+111
-81
lines changed

5 files changed

+111
-81
lines changed

‎redis/asyncio/connection.py

+3-37
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525
)
2626
from urllib.parse import ParseResult, parse_qs, unquote, urlparse
2727

28+
from ..utils import format_error_message
29+
2830
# the functionality is available in 3.11.x but has a major issue before
2931
# 3.11.3. See https://github.com/redis/redis-py/issues/2633
3032
if sys.version_info >= (3, 11, 3):
@@ -315,9 +317,8 @@ async def _connect(self):
315317
def _host_error(self) -> str:
316318
pass
317319

318-
@abstractmethod
319320
def _error_message(self, exception: BaseException) -> str:
320-
pass
321+
return format_error_message(self._host_error(), exception)
321322

322323
async def on_connect(self) -> None:
323324
"""Initialize the connection, authenticate and select a database"""
@@ -702,27 +703,6 @@ async def _connect(self):
702703
def _host_error(self) -> str:
703704
return f"{self.host}:{self.port}"
704705

705-
def _error_message(self, exception: BaseException) -> str:
706-
# args for socket.error can either be (errno, "message")
707-
# or just "message"
708-
709-
host_error = self._host_error()
710-
711-
if not exception.args:
712-
# asyncio has a bug where on Connection reset by peer, the
713-
# exception is not instanciated, so args is empty. This is the
714-
# workaround.
715-
# See: https://github.com/redis/redis-py/issues/2237
716-
# See: https://github.com/python/cpython/issues/94061
717-
return f"Error connecting to {host_error}. Connection reset by peer"
718-
elif len(exception.args) == 1:
719-
return f"Error connecting to {host_error}. {exception.args[0]}."
720-
else:
721-
return (
722-
f"Error {exception.args[0]} connecting to {host_error}. "
723-
f"{exception}."
724-
)
725-
726706

727707
class SSLConnection(Connection):
728708
"""Manages SSL connections to and from the Redis server(s).
@@ -874,20 +854,6 @@ async def _connect(self):
874854
def _host_error(self) -> str:
875855
return self.path
876856

877-
def _error_message(self, exception: BaseException) -> str:
878-
# args for socket.error can either be (errno, "message")
879-
# or just "message"
880-
host_error = self._host_error()
881-
if len(exception.args) == 1:
882-
return (
883-
f"Error connecting to unix socket: {host_error}. {exception.args[0]}."
884-
)
885-
else:
886-
return (
887-
f"Error {exception.args[0]} connecting to unix socket: "
888-
f"{host_error}. {exception.args[1]}."
889-
)
890-
891857

892858
FALSE_STRINGS = ("0", "F", "FALSE", "N", "NO")
893859

‎redis/connection.py

+2-37
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
HIREDIS_AVAILABLE,
3232
HIREDIS_PACK_AVAILABLE,
3333
SSL_AVAILABLE,
34+
format_error_message,
3435
get_lib_version,
3536
str_if_bytes,
3637
)
@@ -311,9 +312,8 @@ def _connect(self):
311312
def _host_error(self):
312313
pass
313314

314-
@abstractmethod
315315
def _error_message(self, exception):
316-
pass
316+
return format_error_message(self._host_error(), exception)
317317

318318
def on_connect(self):
319319
"Initialize the connection, authenticate and select a database"
@@ -642,27 +642,6 @@ def _connect(self):
642642
def _host_error(self):
643643
return f"{self.host}:{self.port}"
644644

645-
def _error_message(self, exception):
646-
# args for socket.error can either be (errno, "message")
647-
# or just "message"
648-
649-
host_error = self._host_error()
650-
651-
if len(exception.args) == 1:
652-
try:
653-
return f"Error connecting to {host_error}. \
654-
{exception.args[0]}."
655-
except AttributeError:
656-
return f"Connection Error: {exception.args[0]}"
657-
else:
658-
try:
659-
return (
660-
f"Error {exception.args[0]} connecting to "
661-
f"{host_error}. {exception.args[1]}."
662-
)
663-
except AttributeError:
664-
return f"Connection Error: {exception.args[0]}"
665-
666645

667646
class SSLConnection(Connection):
668647
"""Manages SSL connections to and from the Redis server(s).
@@ -839,20 +818,6 @@ def _connect(self):
839818
def _host_error(self):
840819
return self.path
841820

842-
def _error_message(self, exception):
843-
# args for socket.error can either be (errno, "message")
844-
# or just "message"
845-
host_error = self._host_error()
846-
if len(exception.args) == 1:
847-
return (
848-
f"Error connecting to unix socket: {host_error}. {exception.args[0]}."
849-
)
850-
else:
851-
return (
852-
f"Error {exception.args[0]} connecting to unix socket: "
853-
f"{host_error}. {exception.args[1]}."
854-
)
855-
856821

857822
FALSE_STRINGS = ("0", "F", "FALSE", "N", "NO")
858823

‎redis/utils.py

+12
Original file line numberDiff line numberDiff line change
@@ -145,3 +145,15 @@ def get_lib_version():
145145
except metadata.PackageNotFoundError:
146146
libver = "99.99.99"
147147
return libver
148+
149+
150+
def format_error_message(host_error: str, exception: BaseException) -> str:
151+
if not exception.args:
152+
return f"Error connecting to {host_error}."
153+
elif len(exception.args) == 1:
154+
return f"Error {exception.args[0]} connecting to {host_error}."
155+
else:
156+
return (
157+
f"Error {exception.args[0]} connecting to {host_error}. "
158+
f"{exception.args[1]}."
159+
)

‎tests/test_asyncio/test_connection.py

+44-7
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@
1212
_AsyncRESPBase,
1313
)
1414
from redis.asyncio import ConnectionPool, Redis
15-
from redis.asyncio.connection import Connection, UnixDomainSocketConnection, parse_url
15+
from redis.asyncio.connection import (
16+
Connection,
17+
SSLConnection,
18+
UnixDomainSocketConnection,
19+
parse_url,
20+
)
1621
from redis.asyncio.retry import Retry
1722
from redis.backoff import NoBackoff
1823
from redis.exceptions import ConnectionError, InvalidResponse, TimeoutError
@@ -493,18 +498,50 @@ async def test_connection_garbage_collection(request):
493498

494499

495500
@pytest.mark.parametrize(
496-
"error, expected_message",
501+
"conn, error, expected_message",
497502
[
498-
(OSError(), "Error connecting to localhost:6379. Connection reset by peer"),
499-
(OSError(12), "Error connecting to localhost:6379. 12."),
503+
(SSLConnection(), OSError(), "Error connecting to localhost:6379."),
504+
(SSLConnection(), OSError(12), "Error 12 connecting to localhost:6379."),
500505
(
506+
SSLConnection(),
501507
OSError(12, "Some Error"),
502-
"Error 12 connecting to localhost:6379. [Errno 12] Some Error.",
508+
"Error 12 connecting to localhost:6379. Some Error.",
509+
),
510+
(
511+
UnixDomainSocketConnection(path="unix:///tmp/redis.sock"),
512+
OSError(),
513+
"Error connecting to unix:///tmp/redis.sock.",
514+
),
515+
(
516+
UnixDomainSocketConnection(path="unix:///tmp/redis.sock"),
517+
OSError(12),
518+
"Error 12 connecting to unix:///tmp/redis.sock.",
519+
),
520+
(
521+
UnixDomainSocketConnection(path="unix:///tmp/redis.sock"),
522+
OSError(12, "Some Error"),
523+
"Error 12 connecting to unix:///tmp/redis.sock. Some Error.",
503524
),
504525
],
505526
)
506-
async def test_connect_error_message(error, expected_message):
527+
async def test_format_error_message(conn, error, expected_message):
507528
"""Test that the _error_message function formats errors correctly"""
508-
conn = Connection()
509529
error_message = conn._error_message(error)
510530
assert error_message == expected_message
531+
532+
533+
async def test_network_connection_failure():
534+
with pytest.raises(ConnectionError) as e:
535+
redis = Redis(host="127.0.0.1", port=9999)
536+
await redis.set("a", "b")
537+
assert str(e.value).startswith("Error 111 connecting to 127.0.0.1:9999. Connect")
538+
539+
540+
async def test_unix_socket_connection_failure():
541+
with pytest.raises(ConnectionError) as e:
542+
redis = Redis(unix_socket_path="unix:///tmp/a.sock")
543+
await redis.set("a", "b")
544+
assert (
545+
str(e.value)
546+
== "Error 2 connecting to unix:///tmp/a.sock. No such file or directory."
547+
)

‎tests/test_connection.py

+50
Original file line numberDiff line numberDiff line change
@@ -296,3 +296,53 @@ def mock_disconnect(_):
296296

297297
assert called == 1
298298
pool.disconnect()
299+
300+
301+
@pytest.mark.parametrize(
302+
"conn, error, expected_message",
303+
[
304+
(SSLConnection(), OSError(), "Error connecting to localhost:6379."),
305+
(SSLConnection(), OSError(12), "Error 12 connecting to localhost:6379."),
306+
(
307+
SSLConnection(),
308+
OSError(12, "Some Error"),
309+
"Error 12 connecting to localhost:6379. Some Error.",
310+
),
311+
(
312+
UnixDomainSocketConnection(path="unix:///tmp/redis.sock"),
313+
OSError(),
314+
"Error connecting to unix:///tmp/redis.sock.",
315+
),
316+
(
317+
UnixDomainSocketConnection(path="unix:///tmp/redis.sock"),
318+
OSError(12),
319+
"Error 12 connecting to unix:///tmp/redis.sock.",
320+
),
321+
(
322+
UnixDomainSocketConnection(path="unix:///tmp/redis.sock"),
323+
OSError(12, "Some Error"),
324+
"Error 12 connecting to unix:///tmp/redis.sock. Some Error.",
325+
),
326+
],
327+
)
328+
def test_format_error_message(conn, error, expected_message):
329+
"""Test that the _error_message function formats errors correctly"""
330+
error_message = conn._error_message(error)
331+
assert error_message == expected_message
332+
333+
334+
def test_network_connection_failure():
335+
with pytest.raises(ConnectionError) as e:
336+
redis = Redis(port=9999)
337+
redis.set("a", "b")
338+
assert str(e.value) == "Error 111 connecting to localhost:9999. Connection refused."
339+
340+
341+
def test_unix_socket_connection_failure():
342+
with pytest.raises(ConnectionError) as e:
343+
redis = Redis(unix_socket_path="unix:///tmp/a.sock")
344+
redis.set("a", "b")
345+
assert (
346+
str(e.value)
347+
== "Error 2 connecting to unix:///tmp/a.sock. No such file or directory."
348+
)

0 commit comments

Comments
 (0)