Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add details to the asyncio connection error message #3211

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add details to the asyncio connection error message #3211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b2d295
Add details to the asyncio connection error message
dmitry-kanev Apr 20, 2024
93eec91
Merge branch 'master' into master
gerzse Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion redis/asyncio/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ def _error_message(self, exception: BaseException) -> str:
else:
return (
f"Error {exception.args[0]} connecting to {host_error}. "
f"{exception.args[0]}."
f"{exception}."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for straightening this out.

This rendering of the error message is done in four places, sync and async, unix and network socket. All four slightly different from each other.

Would you care aligning all four places? Some ideas that come on the spot:

  • pull out the handling at least into the two abstract connection classes, maybe even reuse a helper method between the two abstract classes;
  • if there are no args, return a simple error message, don't even assume it's the "connection reset", there might be other cases too;
  • if there is one arg, use that one;
  • if there are more then one args, use the first one like it is today, and then put exception.arg[1:] at the end of the message, concatenated.

What do you think, does it make sense?

)


Expand Down
18 changes: 18 additions & 0 deletions tests/test_asyncio/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,3 +491,21 @@ async def test_connection_garbage_collection(request):

await client.aclose()
await pool.aclose()


@pytest.mark.parametrize(
"error, expected_message",
[
(OSError(), "Error connecting to localhost:6379. Connection reset by peer"),
(OSError(12), "Error connecting to localhost:6379. 12."),
(
OSError(12, "Some Error"),
"Error 12 connecting to localhost:6379. [Errno 12] Some Error.",
),
],
)
async def test_connect_error_message(error, expected_message):
"""Test that the _error_message function formats errors correctly"""
conn = Connection()
error_message = conn._error_message(error)
assert error_message == expected_message