How should exceptions from underlying client libraries be handled?

[daa]

Suppose I have a consumer for some API and suppose there is some network or server problems resulting in some sort of connection error. Currently ConnectionError from requests or ClientConnectionError from aiohttp are raised and it results in that calling code must know which library is used and it must handle different exceptions in synchronous and asynchronous case, this is rather inconvenient. Situation becomes worse if I make some another adapter, for example with pycurl: interface and calling convention remain the same but very different exceptions are raised.

I can see 2 ways to deal with it:

• define such behaviour to be by design and probably reexport exception classes as attributes of consumer instance or do nothing for simplicity and put the burden on user's code
• define some uplink's exceptions and wrap libraries' errors in them
  However both of these ways may lead to information loss which may be not desirable or may be not.

What are your thoughts about it?

And one more question: suppose I want to handle some exceptions or errors in Consumer to do something with them, for example to wrap them in my custom exception, how can I do it?

[prkumar]

This is a great question. Personally, I prefer an approach that sits somewhere in the middle; I think we can "reexport" and "wrap" client exceptions by introducing a virtual abstract base class (see abc.ABCMeta.register for how to register virtual subclasses).

For instance, our base exception could be HttpError (for the sake of this example, let's say it's defined in the uplink.clients.errors module), and we would register requests.exceptions.RequestException and aiohttp.ClientError as virtual subclasses. Then, users can catch all client related errors using:

try:
	# some code that uses a uplink consumer
except uplink.client.errors.HttpError as error:
	# do something with this error.

Of course, we could do this same thing with other common exceptions, such as ConnectionError.

This way, we support a general solution for client agnostic code with no information loss, and users can still explicitly catch client specific exceptions (e.g., requests.exceptions.RequestException or aiohttp.ClientError) if they want. Further, this behooves anyone that writes a new client adapter to appropriately register their library's exceptions.

@daa - What are your thoughts about this approach?

[prkumar]

@daa - regarding your second question, checkout the decorator error_handler I'm adding in #63. The decorator can apply custom error handling logic across all invocations of request method.

[daa]

I like your idea with virtual subclasses for its clearance and simplicity. However it has global effect and unrelated say RequestException's may be mistakenly treated as uplink's. On the other side it is not worse than current state - code which doesn't know about uplink exceptions is unaffected, and if one wants to mix requests and uplink and behave differently basing on errors he should handle exceptions closely to function call. And I should note that your approach keeps backwards compatibility which is great. So overall it would be strictly an improvement without regression, at least as I can see.

About error_handler - it fulfills my needs, thank you.

[prkumar]

Started working on this in #70.

FYI: @daa

[daa]

Thank you, nice work. It's a pity that approach with virtual subclasses didn't work, it was a nice idea. Exceptions proxying should work for most cases. But I can see one drawback following from that proxies are tuples named like exceptions but not exceptions themselves: one cannot catch several client exceptions in usual manner. For example following code will not work at python3:

try:
      x = cons.method()
except (client_exceptions.ConnectionError, client_exceptions.InvalidUrl):
    ...

It will result in TypeError: catching classes that do not inherit from BaseException is not allowed, one should write except client_exceptions.ConnectionError + client_exceptions.InvalidUrl: instead. Interestingly that code will work for python2, and isinstance(e, (client_exceptions.ConnectionError, client_exceptions.InvalidUrl)) at both Pythons will not complain and work as supposed. However, I don't know how to do better than you've done. Also it's questionable whether such exception handling as in example is reasonable, sane and required at all.

[prkumar]

@daa - Thanks! Hmm... that's an interesting and noteworthy drawback that you have found. In fact, I'm puzzled... why would Python raise a TypeError when except is given a tuple of subtuples. My understanding is that this behavior should be supported.

Firstly, the CPython documentation for PyErr_GivenExceptionMatches, which is the function that ostensibly determines if a raised error matches the excepted condition, boasts that

"If exc is a tuple, all exception types in the tuple (and recursively in subtuples) are searched for a match."

Further, the C function's definition seems to indicate that this is the intended behavior, too. In brief, I wonder if this is a latent defect in CPython? 🤔 It might be worth you filing a bug report through the CPython Issue Tracker, so that they can either update the documentation or fix the bug.

Anyways, I think a possible workaround for us would be to rename the proxy errors to make it clearer that these are sequences/collections of errors. For instance, we would rename ConnectionError to CONNECTION_ERRORS? It looks a little less neat, but what do you think about that?

[prkumar]

Here's some interesting trivia: this ten-year old commit from the CPython repo seems to be the root cause of the behavior difference between how Python 2.7 and 3 does exception handling with tuples. I believe this change may have been unintentional and has gone unnoticed, as the commit is part of the 3.0 release but even the What's New in Python 3.0 documentation doesn't cover it.

[daa]

Actually recursive exception matching was a rather strange feature of Python, and I was surprised when discovered it. Renamed exception collections will really look less neat but at least they'll be honest about their type and supposed usage.

Another way for proxying would be to force client adapters to define some set of exceptions as their attributes and then expose them at Consumer instance. Thus exception handling will look like:

try:
    res = cons.method()
except cons.ConnectionError:
    ....

All such consumer attributes will be exceptions taken from underlying client. But this will require changes in error handler signature - it will have to accept consumer instance to be able to check exceptions. Also these exceptions will not be visible from uplink public api which has downsides. Overall I'm fine with your approach and here just wanted to show an alternative.

[prkumar]

@daa I like your alternative. It appears neater than my workaround, and I think we can make it work. But before we go down that path, I have one final alternative and would appreciate your opinion: on registration of a client exception, we could make the proxy a direct superclass by dynamically appending the proxy exception to the client exceptions class hierarchy. It's a little hacky but it seems to work: I just pushed the change to #70. Let me know what you think!

[daa]

Dynamic superclassing is clever but I'm afraid it's too hacky and may rely on implementation details, and personally I prefer more simple and straightforward code. Also Python data model documentation (https://docs.python.org/3/library/stdtypes.html#special-attributes) tells us that __bases__ is supposed to be read-only attribute even if it can be changed actually but not for built-in exceptions. I'll repeat this is clever trick but it patches unrelated code someone other is responsible for, which may be seen as a problem from maintenance point of view, and articles and stackoverflow answers describing similar technique warn that this should not be normally used.

At all I think problems and hacks arise from two contradictory wishes:

• first, wish to know raised exceptions upfront
• and second, wish to use original exceptions from underlying client which may be not known until consumer configuration
  Upfront visibility of exceptions is useful when we consider for example decorators providing retry functionality, and availability of original exceptions is useful when fine control over process is desired.

At the end I have another idea, may be not so bright as dynamically setting superclass, and involving more code - you may define hierarchy of client exceptions and then dynamically construct proxy classes for real exceptions as inherited from real exception class and corresponding uplink client exception class and then reraise them instead of original. It's not very elegant but sufficiently self-contained.

Also you may look at django if you haven't already - they had similar issue and similar thoughts - https://code.djangoproject.com/ticket/15901 - and decided to wrap exceptions to their own, but they had simpler case - all needed exceptions were listed in pep and libraries conformed to it.

Again, thank you for your work, this issue happened to be rather difficult and non-obvious and involve different tradeoffs.

[prkumar]

@daa - Sorry for taking this long to reply. Last couple weeks have been pretty busy.

You make several fair points. I do appreciate the simplicity of appending our proxy exceptions onto the MRO, but I can't ignore that it muddies the well-defined boundaries between our codebases and the client's. It's also a maintainability headache waiting to happen.

Your idea of dynamically constructing proxy classes sounds like a solid alternative. I've pushed an implementation to #70. Here's a quick overview, taken from the uplink.clients.exceptions module doc:

The workaround employed here maintains the need for client writers to
register their exceptions. For each registered exception, we generate a
subclass that is a child of both the client exception and the proxy
exception to which the writer registered it.

When a client-raised exception is thrown, the client layer attempts to
exchange the client error for a subclass generated in the previous step.
If the exchange is successfully, end-users can catch the subclass
exception using either the proxy exception or the original exception, since
both are superclasses.

This implementation shares a similar API to the previous iteration. Namely, proxy exceptions are defined in the uplink.client.exceptions module and end-users can catch client errors using these proxies or using the original client exceptions.

[daa]

Sorry for delay in response. Code looks good for me, however I noticed that subclasses of registered exceptions will not be wrapped. Consider requests.exceptions.ConnectTimeout - it subclasses requests.exceptions.Timeout which is registered but ConnectTimeout itself is not and when it is raised wrap_exception_if_necessary() method will not handle it. Also ConnectTimeout actually subclasses 2 registered exceptions - ConnectionError and Timeout and if one would like to catch the former with any of the latter some more work needs to be done in wrapping method. The same applies to all exceptions inherited from base registered exception but not registered themselves. So proxying exceptions happens to be more tricky than seemed at first sight.

[prkumar]

@daa - I took another shot at this: #117

[daa]

Thank you, I really appreciate your effort to solve this issue. I like that it is responsibility of an adapter to correctly expose required exceptions, also I like that http client now carries all essential bits of interface to use.

[prkumar]

@daa - No problem! Thank you for providing helpful input and sticking with me throughout the numerous iterations. I'll merge this into develop soon; the change will go-live as part of v0.7.0.