Skip to content

Commit 0be67bf

Browse files
gerzsegerzse
authored
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 04962e0 commit 0be67bf

File tree

5 files changed

+111
-81
lines changed

5 files changed

+111
-81
lines changed

Diff for: ‎redis/asyncio/connection.py

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

30+
from ..utils import format_error_message
31+
3032
# the functionality is available in 3.11.x but has a major issue before
3133
# 3.11.3. See https://github.com/redis/redis-py/issues/2633
3234
if sys.version_info >= (3, 11, 3):
@@ -345,9 +347,8 @@ async def _connect(self):
345347
def _host_error(self) -> str:
346348
pass
347349

348-
@abstractmethod
349350
def _error_message(self, exception: BaseException) -> str:
350-
pass
351+
return format_error_message(self._host_error(), exception)
351352

352353
async def on_connect(self) -> None:
353354
"""Initialize the connection, authenticate and select a database"""
@@ -799,27 +800,6 @@ async def _connect(self):
799800
def _host_error(self) -> str:
800801
return f"{self.host}:{self.port}"
801802

802-
def _error_message(self, exception: BaseException) -> str:
803-
# args for socket.error can either be (errno, "message")
804-
# or just "message"
805-
806-
host_error = self._host_error()
807-
808-
if not exception.args:
809-
# asyncio has a bug where on Connection reset by peer, the
810-
# exception is not instanciated, so args is empty. This is the
811-
# workaround.
812-
# See: https://github.com/redis/redis-py/issues/2237
813-
# See: https://github.com/python/cpython/issues/94061
814-
return f"Error connecting to {host_error}. Connection reset by peer"
815-
elif len(exception.args) == 1:
816-
return f"Error connecting to {host_error}. {exception.args[0]}."
817-
else:
818-
return (
819-
f"Error {exception.args[0]} connecting to {host_error}. "
820-
f"{exception}."
821-
)
822-
823803

824804
class SSLConnection(Connection):
825805
"""Manages SSL connections to and from the Redis server(s).
@@ -971,20 +951,6 @@ async def _connect(self):
971951
def _host_error(self) -> str:
972952
return self.path
973953

974-
def _error_message(self, exception: BaseException) -> str:
975-
# args for socket.error can either be (errno, "message")
976-
# or just "message"
977-
host_error = self._host_error()
978-
if len(exception.args) == 1:
979-
return (
980-
f"Error connecting to unix socket: {host_error}. {exception.args[0]}."
981-
)
982-
else:
983-
return (
984-
f"Error {exception.args[0]} connecting to unix socket: "
985-
f"{host_error}. {exception.args[1]}."
986-
)
987-
988954

989955
FALSE_STRINGS = ("0", "F", "FALSE", "N", "NO")
990956

Diff for: ‎redis/connection.py

+2-37
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
HIREDIS_AVAILABLE,
4040
HIREDIS_PACK_AVAILABLE,
4141
SSL_AVAILABLE,
42+
format_error_message,
4243
get_lib_version,
4344
str_if_bytes,
4445
)
@@ -338,9 +339,8 @@ def _connect(self):
338339
def _host_error(self):
339340
pass
340341

341-
@abstractmethod
342342
def _error_message(self, exception):
343-
pass
343+
return format_error_message(self._host_error(), exception)
344344

345345
def on_connect(self):
346346
"Initialize the connection, authenticate and select a database"
@@ -733,27 +733,6 @@ def _connect(self):
733733
def _host_error(self):
734734
return f"{self.host}:{self.port}"
735735

736-
def _error_message(self, exception):
737-
# args for socket.error can either be (errno, "message")
738-
# or just "message"
739-
740-
host_error = self._host_error()
741-
742-
if len(exception.args) == 1:
743-
try:
744-
return f"Error connecting to {host_error}. \
745-
{exception.args[0]}."
746-
except AttributeError:
747-
return f"Connection Error: {exception.args[0]}"
748-
else:
749-
try:
750-
return (
751-
f"Error {exception.args[0]} connecting to "
752-
f"{host_error}. {exception.args[1]}."
753-
)
754-
except AttributeError:
755-
return f"Connection Error: {exception.args[0]}"
756-
757736

758737
class SSLConnection(Connection):
759738
"""Manages SSL connections to and from the Redis server(s).
@@ -930,20 +909,6 @@ def _connect(self):
930909
def _host_error(self):
931910
return self.path
932911

933-
def _error_message(self, exception):
934-
# args for socket.error can either be (errno, "message")
935-
# or just "message"
936-
host_error = self._host_error()
937-
if len(exception.args) == 1:
938-
return (
939-
f"Error connecting to unix socket: {host_error}. {exception.args[0]}."
940-
)
941-
else:
942-
return (
943-
f"Error {exception.args[0]} connecting to unix socket: "
944-
f"{host_error}. {exception.args[1]}."
945-
)
946-
947912

948913
FALSE_STRINGS = ("0", "F", "FALSE", "N", "NO")
949914

Diff for: ‎redis/utils.py

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

Diff for: ‎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
@@ -494,18 +499,50 @@ async def test_connection_garbage_collection(request):
494499

495500

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

Diff for: ‎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)