We ought to comprehensively revisit our exceptions for 1.0, and make sure we're super happy we've got a public API we're happy to stick by for 1.0.
.request/.response attributes (can we make either of these mandatory in some cases?)See also: #940, #884, #869.
Current hierarchy looks like this...
httpcoreWe're currently synonym'ing these, but we might want to properly map them instead, so we can subclass our base class, and so that we can include a mandatory request=... parameter.
Also, we might want to additionally subclass the stdlib TimeoutError for the timeout types.
ProtocolErrorProxyErrorTimeoutException Exists in httpcore, but not in httpxPoolTimeoutConnectTimeoutReadTimeoutWriteTimeoutNetworkErrorConnectErrorReadErrorWriteErrorCloseErrorDecodingErrorRedirectErrorTooManyRedirectsNotRedirectResponseWe might want to additionally subclass the stdlib RuntimeError for (some of?) these types.
It's actually possible that we might want to just drop some of these from the API completely, and instead use RuntimeError, since not all of these ought to actually be handled exception cases, but are instead unconditionally a "the developer has broken something" case.
StreamErrorRequestBodyUnavailableStreamConsumedResponseNotReadRequestNotReadResponseClosedOther cases...
InvalidURLCookieConflict+1 in favor of mapping the httpcore exceptions to a unified hierarchy of httpx exceptions. I have a library that I recently updated from httpx 0.12 to 0.13 and found that getting the equivalent of except HTTPError was tricky due to the fragmented exception hierarchy.
+1 in favor of mapping the httpcore exceptions to a unified hierarchy of httpx exceptions. I have a library that I recently updated from httpx 0.12 to 0.13 and found that getting the equivalent of
except HTTPErrorwas tricky due to the fragmented exception hierarchy.
I've also run into the same thing. Before, I know that catching HTTPError would also catch TimeoutException. Now, a test case indicated that this isn't the case, and it seems like I'd have to catch a number of exceptions to reproduce the same previous behavior, as @johnanthonyowens said.
In general, it would be nice to have some sort of exception where I know that catching that exception would catch all httpx exceptions and underlying httpcore exceptions. My project is trio-based, where it's really important that certain exceptions are caught and handled properly (or ignored but clearly caught), otherwise other tasks may cancel.
Edit: I saw this comment https://github.com/encode/httpx/pull/884#issuecomment-634749263 which reassures me that catching HTTPError should catch those underlying timeout errors?
I just upgraded a project from httpx 0.12 to 0.13 and found a few things broke since HTTPError is not really the "Base class for all HTTPX exceptions" anymore. I assume this is a bug? I'm fine to drop back to 0.12 for the time being, and I'd rather do that than go find all of the cases in my code where catching HTTPError is no longer sufficient.
Also, it seems that I'm getting httpcore exceptions being raised, not httpx exceptions. I assume this is also a bug?
@iwoloschin As per https://github.com/encode/httpx/pull/884#issuecomment-634749263 yes, any exception leaking from httpcore not wrapped by HTTPX should be considered a bug, which is also what this issue is about.
I'm labelling this as "bug" as well since obviously currently the "HTTPX may raise exceptions that aren't mapped under its base class" is being considered a problem now and should be addressed by reviewing the exception hierarchy.
I'm also wondering if we'd need a "base exception" in httpcore as well (we're subclassing from Exception for most cases there), because it would have made a workaround such as "catch httpx.HTTPError and httpcore.RequestError [or something]" possible.
I think its is probably related. the AsyncClient.get method current throws httpcore._exceptions.ReadTimeout rather than an exception that should be exposed. ( assuming _exceptions is supposed to be private).
I've been reviewing this lately, and I think I've got a clear idea how we could improve things a little.
Here's our current heirarchy...
.raise_for_status.next() without properly checking .is_redirect_response.content without having .read() the stream.content without having .read() the streamresponse.cookies.get(...)☩: Maps with httpcore. Can occur during a .request.
✧: Can occur during a .request.
An important thing to note here is that there's some places where we're drawing the heirarchy along semantic lines, rather than behavioural lines. Ie. both TooManyRedirects and NotRedirectResponse are subclassed as a RedirectError, but behaviourally they're actually entirely different categories - the first cannot be pre-empted by the developer, and depends on the response, the second is the result of a code error. If it's raised it means your code is borken - it isn't ever something you'd want to treat in a catch-and-handle block.
With that in mind, here's how an alternate hierarchy could look...
.raise_for_status.next() without properly checking .is_redirect_responseresponse.cookies.get(...).content without having .read() the stream.content without having .read() the streamNote that in this case we do now have a RequestError class, which encompasses anything that could be raised during a .request() operation. We don't have a general purpose base-of-everything class, since it's not actually a useful thing to catch against.
We can be a bit more precise about exc.request and exc.response in this hierarchy too, and have request= be mandatory for RequestError, and have both request= and response= be mandatory for HTTPStatusError, NotRedirectResponse and CookieConflict. We probably just omit them on the StreamError cases, since those are all the result of programmatic bugs, rather than useful catch-and-handle cases.
There's a couple of minor tweaks we might need beyond that. We'd possibly need a URLParseError that isn't associated with a request, which can be raised if URL() is called directly. Not 100% sure about that yet. We'd probably also need to distinguish two separate classes an internal-only DecodingError which isn't associated with a request, and is raised by the decoder instances, and an external ResponseDecodingError, which would have mandatory request and response arguments, and is what is exposed to the end-user.
I'm milestoning this as 0.14.
That might be a push, I'm not sure, but the rationale for doing so is that post 0.14 everything else we've got left on the table is generally behavioural polishing (eg. charset handling, dropping HSTS, HTTP/2 as optional) rather than API changes.
I'd be okay with us dropping it from the milestone if needed.
I agree that we most likely want this for 0.14. :-)
After https://github.com/encode/httpcore/pull/128 and https://github.com/encode/httpcore/pull/129 we're looking at this...
Now resolved.
Documentation upcoming in https://github.com/encode/httpx/pull/1138
Most helpful comment
I've been reviewing this lately, and I think I've got a clear idea how we could improve things a little.
Here's our current heirarchy...
.raise_for_status.next()without properly checking.is_redirect_response.contentwithout having.read()the stream.contentwithout having.read()the streamresponse.cookies.get(...)☩: Maps with
httpcore. Can occur during a.request.✧: Can occur during a
.request.An important thing to note here is that there's some places where we're drawing the heirarchy along semantic lines, rather than behavioural lines. Ie. both
TooManyRedirectsandNotRedirectResponseare subclassed as aRedirectError, but behaviourally they're actually entirely different categories - the first cannot be pre-empted by the developer, and depends on the response, the second is the result of a code error. If it's raised it means your code is borken - it isn't ever something you'd want to treat in a catch-and-handle block.With that in mind, here's how an alternate hierarchy could look...
.raise_for_status.next()without properly checking.is_redirect_responseresponse.cookies.get(...).contentwithout having.read()the stream.contentwithout having.read()the streamNote that in this case we do now have a
RequestErrorclass, which encompasses anything that could be raised during a.request()operation. We don't have a general purpose base-of-everything class, since it's not actually a useful thing to catch against.We can be a bit more precise about
exc.requestandexc.responsein this hierarchy too, and haverequest=be mandatory forRequestError, and have bothrequest=andresponse=be mandatory forHTTPStatusError,NotRedirectResponseandCookieConflict. We probably just omit them on theStreamErrorcases, since those are all the result of programmatic bugs, rather than useful catch-and-handle cases.There's a couple of minor tweaks we might need beyond that. We'd possibly need a
URLParseErrorthat isn't associated with arequest, which can be raised ifURL()is called directly. Not 100% sure about that yet. We'd probably also need to distinguish two separate classes an internal-onlyDecodingErrorwhich isn't associated with a request, and is raised by the decoder instances, and an externalResponseDecodingError, which would have mandatoryrequestandresponsearguments, and is what is exposed to the end-user.