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

[Feature Request]: raise httpx exception instead of buildin ConnectionResetError and ConnectionError #349

Closed
trim21 opened this issue Sep 16, 2019 · 20 comments · Fixed by #707
Labels
good first issue Good for newcomers requests-compat Issues related to Requests backwards compatibility

Comments

@trim21
Copy link
Contributor

trim21 commented Sep 16, 2019

This is a feature requests instead of bug report.

I'm using requests in my web server to request upstream api. So I can handler all requests.RequestException in my middleware and return a 502 upstream error.

But after I migrated to httpx, httpx doesn't have such exception when issuing a http(s) request, it will raise httpx.ConnectTimeout, buildin ConnectionError and ConnectResetError. And last 2 exception are not only raised by httpx.

Maybe httpx should also wrap ConnectionError and other buildin connection exception?

@trim21 trim21 changed the title raise httpx exception instead of buildin connection error raise httpx exception instead of buildin ConnectionResetError Sep 16, 2019
@trim21 trim21 changed the title raise httpx exception instead of buildin ConnectionResetError raise httpx exception instead of buildin ConnectionResetError and ConnectionError Sep 16, 2019
@florimondmanca
Copy link
Member

Hi @trim21,

I think we're going to need a bit more context to address this issue. :-)

  • What were you trying to do?
  • Can you share a minimal reproducible example?
  • A traceback of the error?

Thanks!

@trim21
Copy link
Contributor Author

trim21 commented Sep 16, 2019

@florimondmanca Sorry, I submit this issue by accident and I'm still writting details.

@trim21 trim21 changed the title raise httpx exception instead of buildin ConnectionResetError and ConnectionError [Feature Request]: raise httpx exception instead of buildin ConnectionResetError and ConnectionError Sep 16, 2019
@florimondmanca
Copy link
Member

Thanks for updating the description. :) I'm not entirely sure on this myself, I'll ping @tomchristie on this one.

@florimondmanca florimondmanca added the requests-compat Issues related to Requests backwards compatibility label Sep 16, 2019
@sethmlarson
Copy link
Contributor

Probably because we don't capture any socket.error or socket.gaierror.

@florimondmanca
Copy link
Member

I remember that Trio already wraps errors such as ConnectionResetError into its own exceptions, eg BrokenResourceError. That means whatever wrapping we do needs to occur at the concurrency backend level. :)

@trim21
Copy link
Contributor Author

trim21 commented Sep 17, 2019

@florimondmanca I'm sorry, I'm not familiar with trio. What's the relation between trio and httpx?

@florimondmanca
Copy link
Member

florimondmanca commented Sep 17, 2019

@trim21 HTTPX supports the async/await syntax, which under the hood requires us to work with a library able to perform asynchronous operations. Right now we only support asyncio (which is part of the standard library), but we’ve got plans for supporting alternatives such as trio - see #120. The « concurrency backend » I mentioned in my comment is an internal interface we use to abstract our code away from the individual async libraries.

I guess the point in my comment was that which exceptions we need to wrap may depend on the async library in use, so we should do the wrapping at the concurrency backend level — provided we do want to wrap those exceptions.

@trim21
Copy link
Contributor Author

trim21 commented Sep 17, 2019

@florimondmanca thanks for your explanation

@trim21
Copy link
Contributor Author

trim21 commented Sep 21, 2019

will this be a feature of 1.0 ?

@sethmlarson
Copy link
Contributor

It'll probably be in before 1.0, just requires a contributor to do the work! :)

@sethmlarson sethmlarson added the good first issue Good for newcomers label Sep 21, 2019
@tomchristie
Copy link
Member

Yeah our guidelines here are probably roughly that all networking exception types should tend to be wrapped up in an HTTPX exception, although we're likely not at that point yet in several areas.

@devdupont
Copy link

devdupont commented Oct 26, 2019

Hope your Hacktoberfest is going well. Can confirm this happens with all connection types. All I need to do is turn off WiFi to replicate with both sync:

>>> import httpx
>>> # With connection
>>> httpx.get("http://httpbin.org/status/200").status_code
200
>>> # Without connection
>>> httpx.get("http://httpbin.org/status/200").status_code
Traceback (most recent call last):
...
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

And async:

>>> import asyncio
>>> import httpx
>>> async def fetch():
...   async with httpx.AsyncClient() as client:
...     return await client.get("http://httpbin.com/status/200")
... 
>>> # With connection
>>> asyncio.run(fetch()).status_code
200
>>> # Without connection
>>> asyncio.run(fetch()).status_code
Traceback (most recent call last):
...
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

@sethmlarson
Copy link
Contributor

@flyinactor91 httpbin.com isn't a website. Do you mean https://httpbin.org?

@trim21
Copy link
Contributor Author

trim21 commented Oct 26, 2019

@sethmlarson He wants httpx to raise socket.gaierror.

@devdupont
Copy link

Lol. Yes I do. So much for typing things in again :)

I'm currently handling generic connection issues via except socket.gaierror for the time being. It works (and is stdlib), but a native httpx exception class or the stdlib ConnectionError would be preferred.

@florimondmanca
Copy link
Member

florimondmanca commented Jan 1, 2020

Did a bit of digging, and here's a list of cases it seems we'd want to wrap as HTTPX exceptions:

  • OSError: "This exception is raised when a system function returns a system-related error, including I/O failures". Also deals with socket.error and its subclasses such as socket.gaierror mentioned earlier in this thread.
  • ConnectionError: built-in base class for connection-related issues (including BrokenPipeError, ConnectionResetError and ConnectionRefusedError).
  • trio.BrokenResourceError: "Raised when an attempt to use a resource fails due to external circumstances" - basically trio's own wrapping of the above exceptions.

Note: asyncio doesn't seem to have network-related exceptions that we should handle -- see asyncio exceptions. (There's IncompleteReadError, but it's raised by reader methods we don't use.)

@tomchristie
Copy link
Member

@florimondmanca Fantastic, thanks! It might be good to also cross-reference against exactly what exception heirarchy/wrapping users get with urllib3/requests.

@florimondmanca
Copy link
Member

florimondmanca commented Jan 1, 2020

@tomchristie Sure! So it turns out to be a bit messy, but here's what I found…

urllib3.exceptions

There's a generic ProtocolError (previously named ConnectionError), but it's not a consistent base class for network-related exceptions. Eg. NewConnectionError isn't a subclass of ConnectionError, although it's wrapper for ConnectionRefusedError which is a subclass of the built-in ConnectionError.

Some finer-grained network-related exceptions exists as well:

  • urllib3.exceptions.NewConnectionError: a wrapper for ConnectionRefusedError
  • urllib3.exceptions.IncompleteRead: a wrapper for httplib.IncompleteRead

requests.exceptions

Requests uses a wrap-all requests.exceptions.ConnectionError. Most of the exception handling seems to take place in HTTPAdapter.send(). The adapter catches various urllib3 exceptions raised by conn.urlopen() and other calls, and raises an appropriate subclass of requests.exceptions.ConnectionError, e.g. ProxyError, SSLError, or ConnectionError itself.


I think going the route a wrap-all exception in the client's .send() method sounds sensible enough. I wouldn't follow Requests' naming convention though, because ConnectionError is a built-in. So e.g. NetworkError.

@florimondmanca
Copy link
Member

I think going the route a wrap-all exception in the client's .send() method sounds sensible enough.

The only tricky bit is streaming responses, because in that case we're doing I/O outside of .send().

Which means we may want to perform wrapping inside Response.aiter_raw() and Response.aclose() as well?

@tomchristie
Copy link
Member

Really we want to wrap as close to the exception as possible.
Ie. Specifying what exceptions we expect from our backend .read(), .write() and connection methods, and making sure we're wrapping at that level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers requests-compat Issues related to Requests backwards compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants