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.

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

httpcore/backends/trio: map OSError exceptions #543

Merged
merged 1 commit into from May 11, 2022

Conversation

vfazio
Copy link
Contributor

@vfazio vfazio commented Apr 27, 2022

Closes #544

The underlying socket implementations can raise OSError for situations
such as getaddrinfo failing to resolve an address.

Previously, these errors would not be mapped to httpcore exception types
for the trio backend.

Now, map OSError exceptions in the trio backend to ConnectError.

This will make trio consistent with the asyncio backend.

Signed-off-by: Vincent Fazio vfazio@xes-inc.com

The underlying socket implementations can raise OSError for situations
such as `getaddrinfo` failing to resolve an address.

Previously, these errors would not be mapped to httpcore exception types
for the trio backend.

Now, map OSError exceptions in the trio backend to ConnectError.

This will make trio consistent with the asyncio backend.

Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
@vfazio
Copy link
Contributor Author

vfazio commented May 10, 2022

@tomchristie @michaeloliverx Just tagging since I saw you two were the last to touch this file. If there are concerns, please let me know.

@florimondmanca
Copy link
Member

florimondmanca commented May 10, 2022

@vfazio Is there a simple reproduction case of when this OSError used to occur? Non-existent domain name? Edit: woops, just saw this refers to issue #544.

I'm wondering whether this should be treated as an httpcore issue, or a trio issue: shouldn't trio be handling it and wrapping it in a ConnectError or something?

@vfazio
Copy link
Contributor Author

vfazio commented May 10, 2022

@vfazio Is there a simple reproduction case of when this OSError used to occur? Non-existent domain name? Edit: woops, just saw this refers to issue #544.

I'm wondering whether this should be treated as an httpcore issue, or a trio issue: shouldn't trio be handling it and wrapping it in a ConnectError or something?

@florimondmanca asyncio and trio raise an OSError when a hostname fails to resolve and the solution for asyncio, apparently, was to map those errors to ConnectError so i'm just bringing trio up to parity here i believe

https://github.com/encode/httpcore/blob/master/httpcore/backends/asyncio.py#L104

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@florimondmanca florimondmanca merged commit 15d8093 into encode:master May 11, 2022
@tomchristie tomchristie mentioned this pull request May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trio backend doesn't map OSError exceptions
2 participants