[openstack-dev] [Review] Use of exception for non-exceptional cases

Doug Hellmann doug.hellmann at dreamhost.com
Fri Jul 12 22:53:38 UTC 2013


On Fri, Jul 12, 2013 at 2:40 PM, Brant Knudson <blk at acm.org> wrote:

>
> Somewhat offtopic, but others might find it interesting. On another
> project I used a different technique for exceptions, where the original
> exception is "enhanced" with new fields as it propagates up the stack to
> where it's handled. In this way all the information needed to handle the
> exception is available (even if it's just to log it).
>
> Often there's some information that would be useful for the exception
> handler that isn't available at the place where the exception is raised.
> The classic example is an error writing to a file where you'd like to know
> the filename but all you've got is the file descriptor. An example from an
> OpenStack REST API might be that you get an exception and you'd like to log
> a message that includes the resource path, but the resource path isn't
> known at this point.
>
> So rather than logging the exception when it happens, enhance the
> exception, re-raise it, and only once it's got all the information where
> you can print out a useful log message you log it.
>
> Here's a short example to illustrate. An exception is raised by f1(), but
> at this point we don't know the status code that the server should respond
> with or the request path which would be useful in a log message. These bits
> of information are added as the exception propagates and then where it's
> caught we've got all the information for a useful log message.
>
> def f1():
>   raise Exception('something')
>
>
> def f2():
>   try:
>     f1()
>   except Exception as e:
>     e.status_code = 403
>     raise
>
>
> def do_tokens():
>   try:
>     f2()
>   except Exception as e:
>     e.req_url = '/v3/tokens'
>     raise
>
>
> status_code = 200
> try:
>   do_tokens()
> except Exception as e:
>   status_code = getattr(e, 'status_code', 500)
>   req_url = getattr(e, 'req_url', 'unknown')
>
> if status_code != 200:
>   print 'Got %s from %s' % (status_code, req_url)
>   # build an error document for the response.
>


One problem with this approach is it spreads knowledge of the error
construction up and down the stack through different layers of the
application, and that brings with it assumptions about the implementation
at the different layers. For example, should the application code above
know that do_tokens() is making web requests to URLs? Why does it need that
information?

SRP-ly,
Doug



>
>
>
> On Fri, Jul 12, 2013 at 12:25 PM, Nachi Ueno <nachi at ntti3.com> wrote:
>
>> Hi folks
>>
>> > Monty
>> Thank you for your adding good topic for this.
>>
>> I agree, hiding true cause by exception get hard to debug
>>
>> For, example, Sqlalchemy gives me this error for general sql errors..
>> "exceptions must be old-style classes or derived from BaseException, not
>> str"
>>
>> (It should be hidden from REST API users though)
>>
>> Also, I agree with you about log policies.
>> sometimes 404 get warn log..
>>
>> IMO, The log more than warn level should help Operators.
>> also If the exception is truly, the exceptional case which needs help
>> of operators.
>> It should be logged as error level.
>>
>> Thanks
>> Nachi
>>
>>
>>
>> 2013/7/12 Dolph Mathews <dolph.mathews at gmail.com>:
>> >
>> > On Fri, Jul 12, 2013 at 10:09 AM, Monty Taylor <mordred at inaugust.com>
>> wrote:
>> >>
>> >>
>> >>
>> >> On 07/11/2013 05:20 AM, Mark McLoughlin wrote:
>> >> > On Wed, 2013-07-10 at 19:49 -0400, Monty Taylor wrote:
>> >> >> I'd like top-post and hijack this thread for another exception
>> related
>> >> >> thing:
>> >> >>
>> >> >> a) Anyone writing code such as:
>> >> >>
>> >> >> try:
>> >> >>   blah()
>> >> >> except SomeException:
>> >> >>   raise SomeOtherExceptionLeavingOutStackContextFromSomeException
>> >> >>
>> >> >> should be mocked ruthlessly.
>> >> >
>> >> > Ok, mock me ruthlessly then.
>> >>
>> >> Mock. Mock. Mock. Mock.
>> >>
>> >> > Part of designing any API is specifying what predictable exceptions
>> it
>> >> > will raise. For any predictable error condition, you don't want
>> callers
>> >> > to have to catch random exceptions from the underlying libraries you
>> >> > might be calling into.
>> >>
>> >> Absolutely. But you don't want to go so overboard that you remove the
>> >> ability for a developer to debug it. More importantly, INSIDE of server
>> >> code, we're not designing fun apis for the consumption of people we
>> >> don't know. There is clearly an API, but we are the consumers of our
>> own
>> >> API.
>> >
>> >
>> > Lies! Write everything to be intuitive for new contributors or we won't
>> have
>> > any :(
>> >
>> >>
>> >>
>> >> > Say if I was designing an image downloading API, I'd do something
>> like
>> >> > this:
>> >> >
>> >> >   https://gist.github.com/markmc/5973868
>> >> >
>> >> > Assume there's a tonne more stuff that the API would do. You don't
>> want
>> >> > callers to have to catch socket.error exceptions and whatever other
>> >> > exceptions might be thrown.
>> >> >
>> >> > That works out as:
>> >> >
>> >> >   Traceback (most recent call last):
>> >> >     File "t.py", line 20, in <module>
>> >> >       download_image('localhost', 3366, 'foobar')
>> >> >     File "t.py", line 18, in download_image
>> >> >       raise ImageDownloadFailure(host, port, path, e.strerror)
>> >> >   __main__.ImageDownloadFailure: Failed to download foobar from
>> >> > localhost:3366: Connection refused
>> >> >
>> >> > Which is a pretty clear exception.
>> >>
>> >> Right, but what if the message from the exception does not give you
>> >> enough information to walk down the stack and see it...
>> >>
>> >> Also, I have more than once seen:
>> >>
>> >> class MyIOError(Exception):
>> >>     pass
>> >>
>> >> try:
>> >>     s = socket.create_connection((host, port))
>> >> except socket.error:
>> >>     raise MyIOError("something went wrong!")
>> >>
>> >> Which is an extreme example of where my rage comes from, but not a made
>> >> up example. I have, actually, seen code exactly like that - in Nova.
>> >>
>> >> BTW - I'd have download_image return None if it can't download the
>> >> image, and I'd have it either deal with the socket.error or not locally
>> >> at the source. But now we're nitpicking.
>> >>
>> >> > But I think what you're saying is missing is the stack trace from the
>> >> > underlying exception.
>> >>
>> >> YES - and I'll let David's response respond to the details of the
>> rest...
>> >>
>> >> But essentially, what I was really mocking earlier is throwing a new
>> >> exception in such a way that you lose the exception propagation path.
>> So
>> >> if you throw an exception inside of an except block, you should be sure
>> >> that you're passing on all of the info, because if a developer gets an
>> >> exception, it's quite likely that they want to know how to fix it. :)
>> >>
>> >> > As I understood it, Python doesn't have a way of chaining exceptions
>> >> > like this but e.g. Java does. A little bit more poking right now
>> shows
>> >> > up this:
>> >> >
>> >> >   http://www.python.org/dev/peps/pep-3134/
>> >> >
>> >> > i.e. we can't do the right thing until Python 3, where we'd do:
>> >> >
>> >> >  def download_image(host, port, path):
>> >> >      try:
>> >> >          s = socket.create_connection((host, port))
>> >> >      except socket.error as e:
>> >> >          raise ImageDownloadFailure(host, port, path, e.strerror)
>> from e
>> >>
>> >> This will certainly be cleaner to write and read.
>> >>
>> >> > I haven't read the PEP in detail yet, though.
>> >> >
>> >> > Cheers,
>> >> > Mark.
>> >> >
>> >> >
>> >> > _______________________________________________
>> >> > OpenStack-dev mailing list
>> >> > OpenStack-dev at lists.openstack.org
>> >> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> >> >
>> >>
>> >> _______________________________________________
>> >> OpenStack-dev mailing list
>> >> OpenStack-dev at lists.openstack.org
>> >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> >
>> >
>> >
>> >
>> > --
>> >
>> > -Dolph
>> >
>> > _______________________________________________
>> > OpenStack-dev mailing list
>> > OpenStack-dev at lists.openstack.org
>> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> >
>>
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20130712/38ecae6b/attachment.html>


More information about the OpenStack-dev mailing list