Repository navigation
Exceptions - Documentation, Hierarchy & Naming. #949
Description
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.
- Naming.
- The hierarchy.
- Decent documentation.
.request/.responseattributes (can we make either of these mandatory in some cases?)
Activity
lovelydinosaur commentedon May 13, 2020
Current hierarchy looks like this...
Transport Errors, mapped from
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
stdlibTimeoutError for the timeout types.ProtocolErrorProxyErrorTimeoutExceptionExists inhttpcore, but not inhttpxPoolTimeoutConnectTimeoutReadTimeoutWriteTimeoutNetworkErrorConnectErrorReadErrorWriteErrorCloseErrorContent Errors
DecodingErrorRedirect Errors
RedirectErrorTooManyRedirectsNotRedirectResponseStream Errors
We might want to additionally subclass the
stdlibRuntimeError 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...
InvalidURLCookieConflictfix(httpx): exception handling have changed
johnanthonyowens commentedon May 31, 2020
+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.MicahLyle commentedon Jun 1, 2020
I've also run into the same thing. Before, I know that catching
HTTPErrorwould also catchTimeoutException. 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
httpxexceptions and underlyinghttpcoreexceptions. My project istrio-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 #884 (comment) which reassures me that catching
HTTPErrorshould catch those underlying timeout errors?iwoloschin commentedon Jun 12, 2020
I just upgraded a project from httpx 0.12 to 0.13 and found a few things broke since
HTTPErroris 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 catchingHTTPErroris no longer sufficient.Also, it seems that I'm getting httpcore exceptions being raised, not httpx exceptions. I assume this is also a bug?
florimondmanca commentedon Jun 15, 2020
@iwoloschin As per #884 (comment) yes, any exception leaking from
httpcorenot wrapped by HTTPX should be considered a bug, which is also what this issue is about.florimondmanca commentedon Jun 15, 2020
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
httpcoreas well (we're subclassing fromExceptionfor most cases there), because it would have made a workaround such as "catchhttpx.HTTPErrorandhttpcore.RequestError[or something]" possible.martinka commentedon Jun 18, 2020
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).
lovelydinosaur commentedon Jul 21, 2020
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.lovelydinosaur commentedon Jul 27, 2020
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.
florimondmanca commentedon Jul 27, 2020
I agree that we most likely want this for 0.14. :-)
lovelydinosaur commentedon Aug 2, 2020
After encode/httpcore#128 and encode/httpcore#129 we're looking at this...
· ConnectTimeout
· ReadTimeout
· WriteTimeout
· PoolTimeout
· ConnectError
· ReadError
· WriteError
· CloseError
· LocalProtocolError
· RemoteProtocolError
lovelydinosaur commentedon Aug 6, 2020
Now resolved.
Documentation upcoming in #1138